Destroy stream on exceeding maxContentLength (fixes #1098) #1485

resure · 2018-04-15T14:59:31Z

Currently, axios won't destroy download stream on exceeding maxContentLength, which in some cases can lead to high cpu usage and subsequent denial of service.

Here is how it looks (200 MB file, limit is 20 MB):

   ticks parent  name
  61777   81.0%  /lib/x86_64-linux-gnu/libc-2.23.so
  61542   99.6%    LazyCompile: *Buffer.concat buffer.js:423:25
  61437   99.8%      Function: ~handleStreamData /home/resure/something/node_modules/axios/lib/adapters/http.js:165:52
  61437  100.0%        Function: ~emitOne events.js:114:17
  61437  100.0%          Function: ~emit events.js:156:44
  61437  100.0%            Function: ~addChunk _stream_readable.js:261:18

It almost hangs nodejs process for ~30 seconds, spending all that ticks on handling already rejected download.

This PR adds stream.destroy() (suggested in #1098), which is being called right before throwing an error about size limit.

dinvlad · 2019-03-09T23:18:08Z

Are there updates on when this fix will be merged?

quocnguyen · 2019-03-21T04:48:39Z

Hi @rubennorte could you take a look ?

sdotson · 2019-04-24T18:45:31Z

Update on this one? https://snyk.io/test/npm/axios/0.18.0

rajivshah3 · 2019-05-03T03:16:28Z

@emilyemorehouse would you be able to review this when you get a chance? The DoS vulnerability was recently published on Snyk (https://snyk.io/vuln/SNYK-JS-AXIOS-174505) and may be causing some CI pipelines to fail

dinvlad · 2019-05-03T04:08:55Z

Both Snyk and SourceClear (SC had it for quite some time): https://www.sourceclear.com/vulnerability-database/vulnerabilities/6130

Luidog · 2019-05-03T09:58:37Z

Hello,

Please correct me if I am wrong, but it looks like all that is needed here is a one line change that was submitted via a PR more than a year ago. It also looks like all tests have passed on that PR.

I am very appreciative of the work that has gone into this project. I have quite a few projects of my own that depend on the awesome work contributed here. If there is additional work that needed to be done for this I would be happy and proud to pitch in.

Is there a way we can get this PR merged and released to close the security vulnerability? I would really like to continue to use this project, but I have a responsibility to ensure that the projects that I maintain are free from vulnerabilities.

Thank you for the continued efforts on this project and please let me know if there is something I can contribute to help close the vulnerability and add value to this project.

spilist · 2019-05-09T01:11:05Z

Glad to see this is merged. Does anybody know when would be the next stable release?

gabrielkoo · 2019-05-09T02:17:00Z

The latest build failed: https://travis-ci.org/axios/axios/jobs/529505165

rajivshah3 · 2019-05-09T03:31:48Z

There's a nested dependency with native code that doesn't seem to support Node 12. I'll make a PR to lower the Node version that the CI uses so that the rest of the build can run

Edit: Opened PR #2138

emilyemorehouse · 2019-05-09T17:33:51Z

As soon as CI is green, I'll issue a release!

hughns · 2019-05-10T12:05:53Z

We've tried this code and there appears to be some situations where stream is not yet defined resulting in errors like this:

{
    "errorMessage": "stream is not defined",
    "errorType": "ReferenceError",
    "stackTrace": [
        "dispatchHttpRequest (/var/task/node_modules/axios/lib/adapters/http.js:219:13)",
        "new Promise (<anonymous>)",
        "httpAdapter (/var/task/node_modules/axios/lib/adapters/http.js:18:10)",
        "dispatchRequest (/var/task/node_modules/axios/lib/core/dispatchRequest.js:59:10)",
        "<anonymous>",
        "process._tickDomainCallback (internal/process/next_tick.js:228:7)"
    ]
}

@emilyemorehouse i would not recommend releasing until this is addressed.

Luidog · 2019-05-10T12:57:33Z

I am sorry to say that I have experienced this issue as well:

(node:4825) UnhandledPromiseRejectionWarning: ReferenceError: stream is not defined at dispatchHttpRequest (/home/travis/build/Luidog/fms-api-client/node_modules/axios/lib/adapters/http.js:219:13) at new Promise (<anonymous>) at httpAdapter (/home/travis/build/Luidog/fms-api-client/node_modules/axios/lib/adapters/http.js:18:10) at dispatchRequest (/home/travis/build/Luidog/fms-api-client/node_modules/axios/lib/core/dispatchRequest.js:59:10) at processTicksAndRejections (internal/process/task_queues.js:89:5)

mikecousins · 2019-05-10T23:06:13Z

Can we just wrap it in a null check before destroying?

rajivshah3 · 2019-05-11T19:50:48Z

@Luidog @hughns out of curiosity I looked into this, and it looks like there's something wrong with the patch from Snyk if you try to use it with axios 0.18.0. If you look in your node_modules (after allowing Snyk to apply the patch) the changes are not applied in the correct place since 0.18.0 differs from master:

if (config.responseType === 'stream') {
        response.data = stream;
        settle(resolve, reject, response);
      } else {
        var responseBuffer = [];
        stream.on('data', function handleStreamData(chunk) {
          responseBuffer.push(chunk);

          // make sure the content length is not over the maxContentLength if specified
          if (config.maxContentLength > -1 && Buffer.concat(responseBuffer).length > config.maxContentLength) {
            // stream.destroy() should have been added here
            reject(createError('maxContentLength size of ' + config.maxContentLength + ' exceeded',
              config, null, lastRequest));
          }
        });

        stream.on('error', function handleStreamError(err) {
          if (req.aborted) return;
          reject(enhanceError(err, config, null, lastRequest));
        });

        stream.on('end', function handleStreamEnd() {
          var responseData = Buffer.concat(responseBuffer);
          if (config.responseType !== 'arraybuffer') {
            responseData = responseData.toString('utf8');
          }

          response.data = responseData;
          settle(resolve, reject, response);
        });
      }
    });

    // Handle errors
    req.on('error', function handleRequestError(err) {
      if (req.aborted) return;
      reject(enhanceError(err, config, null, req));
    });

    // Handle request timeout
    if (config.timeout && !timer) {
      timer = setTimeout(function handleRequestTimeout() {
        req.abort();
        reject(createError('timeout of ' + config.timeout + 'ms exceeded', config, 'ECONNABORTED', req));
      }, config.timeout);
    }
            stream.destroy();  // <-- incorrectly added here

    if (config.cancelToken) {
      // Handle cancellation
      config.cancelToken.promise.then(function onCanceled(cancel) {
        if (req.aborted) return;

        req.abort();
        reject(cancel);
      });
    }

So it seems like it's an issue with the Snyk patch, not axios itself. I sent a message to their support team to let them know

lirantal · 2019-05-12T08:55:50Z

Thanks @rajivshah3, working on remediating it.

@hughns @Luidog have you experienced the no stream defined from applying the snyk patch or from the recent change merged to this repo?

lirantal · 2019-05-12T18:05:09Z

One more ping @rajivshah3 @hughns @Luidog - not sure which one of you uses the Snyk patch but heads up that we've updated it a couple of hours ago and if you re-run snyk protect it should download the new version (or in any other issue you could re-apply the patch).

hughns · 2019-05-12T20:39:41Z

@rajivshah3 Well spotted! I should have added that this was via snyk (and that I have raised a ticket with snyk support too). We haven't tested with the 0.19.0-beta.1 release yet.

@lirantal Yes, we saw the issue after the snyk patch was applied. The support ticket is https://support.snyk.io/hc/en-us/requests/445

rajivshah3 · 2019-05-12T20:48:11Z

Thanks for the quick response @lirantal ! The patch looks good now

lirantal · 2019-05-12T20:49:02Z

Great to hear, thanks for the update both!

Luidog · 2019-05-13T14:25:36Z

@lirantal, I did indeed used the original snyk patch which caused the test failures. I have rerun the snyk protect and now the patch is applied correctly. Thank you for following up on this.

lirantal · 2019-05-13T14:36:09Z

@Luidog great news, thanks!

any chance you or @hughns have a way to reproduce the problem for the undefined stream variable that you experienced?

schmod · 2019-05-13T20:42:12Z

Apologies if it's veering offtopic for this thread, but is there a mechanism to tell the Snyk folks when an issue has been remediated in a new release?

lirantal · 2019-05-13T20:51:36Z

@schmod you can always give us a ping through support@snyk.io or security@snyk.io. I'm happy to also take this offline with you through twitter (I'm at https://www.twitter.com/liran_tal) and be a source of contact or any other security related help I can provide

Destroy stream on exceeding maxContentLength (fixes #1098)

79a1ed2

Merge branch 'master' into master

1791bed

morleyzhi mentioned this pull request Apr 29, 2019

DoS in Axios stellar/js-stellar-sdk#268

Closed

igorshubovych mentioned this pull request May 4, 2019

Project dead? #1965

Closed

NiklasWagner approved these changes May 6, 2019

View reviewed changes

emilyemorehouse merged commit 0d4fca0 into axios:master May 7, 2019

freezy added a commit to vpdb/server that referenced this pull request May 11, 2019

chore(snyk): Remove patch that breaks axios (see axios/axios#1485).

31d93ad

This was referenced Apr 30, 2020

[Snyk] Upgrade axios from 0.18.0 to 0.19.2 leonyork/cognito-oauth2-demo#3

Open

[Snyk] Upgrade axios from 0.18.1 to 0.19.2 absalomedia/paperless-desktop#53

Open

axios locked and limited conversation to collaborators May 3, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Destroy stream on exceeding maxContentLength (fixes #1098) #1485

Destroy stream on exceeding maxContentLength (fixes #1098) #1485

resure commented Apr 15, 2018 •

edited

dinvlad commented Mar 9, 2019

quocnguyen commented Mar 21, 2019

sdotson commented Apr 24, 2019 •

edited

rajivshah3 commented May 3, 2019 •

edited

dinvlad commented May 3, 2019 •

edited

Luidog commented May 3, 2019 •

edited

spilist commented May 9, 2019

gabrielkoo commented May 9, 2019

rajivshah3 commented May 9, 2019 •

edited

emilyemorehouse commented May 9, 2019

hughns commented May 10, 2019

Luidog commented May 10, 2019

mikecousins commented May 10, 2019

rajivshah3 commented May 11, 2019 •

edited

lirantal commented May 12, 2019

lirantal commented May 12, 2019

hughns commented May 12, 2019

rajivshah3 commented May 12, 2019

lirantal commented May 12, 2019

Luidog commented May 13, 2019 •

edited

lirantal commented May 13, 2019

schmod commented May 13, 2019 •

edited

lirantal commented May 13, 2019

Navigation Menu

Destroy stream on exceeding maxContentLength (fixes #1098) #1485

Destroy stream on exceeding maxContentLength (fixes #1098) #1485

Conversation

resure commented Apr 15, 2018 • edited

dinvlad commented Mar 9, 2019

quocnguyen commented Mar 21, 2019

sdotson commented Apr 24, 2019 • edited

rajivshah3 commented May 3, 2019 • edited

dinvlad commented May 3, 2019 • edited

Luidog commented May 3, 2019 • edited

spilist commented May 9, 2019

gabrielkoo commented May 9, 2019

rajivshah3 commented May 9, 2019 • edited

emilyemorehouse commented May 9, 2019

hughns commented May 10, 2019

Luidog commented May 10, 2019

mikecousins commented May 10, 2019

rajivshah3 commented May 11, 2019 • edited

lirantal commented May 12, 2019

lirantal commented May 12, 2019

hughns commented May 12, 2019

rajivshah3 commented May 12, 2019

lirantal commented May 12, 2019

Luidog commented May 13, 2019 • edited

lirantal commented May 13, 2019

schmod commented May 13, 2019 • edited

lirantal commented May 13, 2019

resure commented Apr 15, 2018 •

edited

sdotson commented Apr 24, 2019 •

edited

rajivshah3 commented May 3, 2019 •

edited

dinvlad commented May 3, 2019 •

edited

Luidog commented May 3, 2019 •

edited

rajivshah3 commented May 9, 2019 •

edited

rajivshah3 commented May 11, 2019 •

edited

Luidog commented May 13, 2019 •

edited

schmod commented May 13, 2019 •

edited