Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 2 commits into from May 7, 2019

Conversation

Projects
None yet
@resure
Copy link
Contributor

commented Apr 15, 2018

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

This comment has been minimized.

Copy link

commented Mar 9, 2019

Are there updates on when this fix will be merged?

@quocnguyen

This comment has been minimized.

Copy link

commented Mar 21, 2019

Hi @rubennorte could you take a look ?

@sdotson

This comment has been minimized.

Copy link

commented Apr 24, 2019

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

@morleyzhi morleyzhi referenced this pull request Apr 29, 2019

Open

DoS in Axios #268

@rajivshah3

This comment has been minimized.

Copy link

commented May 3, 2019

@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

This comment has been minimized.

Copy link

commented May 3, 2019

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

@Luidog

This comment has been minimized.

Copy link

commented May 3, 2019

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.

@igorshubovych igorshubovych referenced this pull request May 4, 2019

Open

Project dead? #1965

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

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@spilist

This comment has been minimized.

Copy link

commented May 9, 2019

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

@gabrielkoo

This comment has been minimized.

Copy link

commented May 9, 2019

@rajivshah3

This comment has been minimized.

Copy link

commented May 9, 2019

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

This comment has been minimized.

Copy link
Member

commented May 9, 2019

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

@hughns

This comment has been minimized.

Copy link

commented May 10, 2019

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

This comment has been minimized.

Copy link

commented May 10, 2019

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

This comment has been minimized.

Copy link

commented May 10, 2019

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

@rajivshah3

This comment has been minimized.

Copy link

commented May 11, 2019

@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

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

@lirantal

This comment has been minimized.

Copy link

commented May 12, 2019

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

This comment has been minimized.

Copy link

commented May 12, 2019

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

This comment has been minimized.

Copy link

commented May 12, 2019

@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

This comment has been minimized.

Copy link

commented May 12, 2019

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

@lirantal

This comment has been minimized.

Copy link

commented May 12, 2019

Great to hear, thanks for the update both!

@Luidog

This comment has been minimized.

Copy link

commented May 13, 2019

@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

This comment has been minimized.

Copy link

commented May 13, 2019

@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

This comment has been minimized.

Copy link

commented May 13, 2019

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

This comment has been minimized.

Copy link

commented May 13, 2019

@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

@NicoZelaya

This comment has been minimized.

Copy link

commented May 15, 2019

Where does this fix stand in terms of a stable release? Is it feasible to have it released soon? (With the correct fix of course).

I can help work on it if needed, but we would need to get rid of axios otherwise on an open source SDK I'm actively maintaining (which I would prefer to avoid, it's a great library!)

@rajivshah3

This comment has been minimized.

Copy link

commented May 15, 2019

I think @emilyemorehouse said a new release can be created once CI is green. The problem is that it's failing due to an unrelated issue where a dependency won't build on the current version of NodeJS. I fixed it in #2138 but it hasn't been merged yet

@Luidog

This comment has been minimized.

Copy link

commented May 15, 2019

@lirantal I will send you a reproduction via email - security@snyk.io

@lirantal

This comment has been minimized.

Copy link

commented May 18, 2019

thanks @Luidog, appreciate it!

@vimal1083

This comment has been minimized.

Copy link

commented May 21, 2019

When would be the next release?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.