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

crypto: clear error stack in ECDH::BufferToPoint #13275

Closed
wants to merge 1 commit into from
Closed

crypto: clear error stack in ECDH::BufferToPoint #13275

wants to merge 1 commit into from

Conversation

rfk
Copy link
Contributor

@rfk rfk commented May 29, 2017

The ECDH::BufferToPoint function was not clearing the error stack on failure, and the leftover error state could cause subsequent (unrelated) signing operations to fail. This updates the function to use existing error-clearing helpers and seems to resolve the problem for me.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • commit message follows commit guidelines
Affected core subsystem(s)
  • crypto

@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. crypto Issues and PRs related to the crypto subsystem. labels May 29, 2017
'pQhByd5eyj3lgZ7m7jbchtdgyOF8Io/1ng==\n' +
'-----END EC PRIVATE KEY-----';
assert.ok(crypto.createSign('SHA256').sign(ecPrivateKey),
'signing should not throw');
Copy link
Member

@Trott Trott May 29, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe better to use this instead?:

assert.doesNotThrow(() => {
  crypto.createSign('SHA256').sign(ecPrivateKey);
});

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks, will update

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(Although I guess that doesn't check that you get a truthy return value. Could be added as an assertion to the function if it's important.)

@Trott
Copy link
Member

Trott commented May 29, 2017

/cc @nodejs/crypto

Copy link
Member

@indutny indutny left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@@ -5109,6 +5109,8 @@ EC_POINT* ECDH::BufferToPoint(char* data, size_t len) {
return nullptr;
}

MarkPopErrorOnReturn mark_pop_error_on_return;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's not wrong but I'd put this in ECDH::ComputeSecret() and ECDH::SetPublicKey(), the binding functions that call ECDH::BufferToPoint().

Maybe we should simply make it a rule that binding functions must restore the error stack.

curve.generateKeys();
try {
curve.computeSecret(invalidKey);
assert(false, 'key generation should have failed');
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This throws an exception so you still end up in the catch block. The pattern tests usually use is this:

let gotError = false;
try {
  // ...
} catch (e) {
  gotError = true;
  // ...
}
assert(gotError);

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The problem with assert.throws() in this case is that the test case is actually in the catch block. The pattern @bnoordhuis lays out will work as will the way it is currently done in this PR.

Copy link
Contributor

@sam-github sam-github May 29, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But the test case doesn't have to be in the catch block, or am I just missing something?

assert.throws(() => curve.computeSecret(invalidKey));
const ecPrivateKey = ...;
assert.doesNotThrow(() => crypto.createSign('SHA256').sign(ecPrivateKey));

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like this last one, thanks for the suggestion.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@sam-github Ah, yes, you are right.

Functions that call `ECDH::BufferToPoint` were not clearing the
error stack on failure, so an invalid key could leave leftover
error state and cause subsequent (unrelated) signing operations
to fail.
@rfk
Copy link
Contributor Author

rfk commented May 29, 2017

Thanks all, rebased to latest master and updated based on @bnoordhuis @sam-github feedback.

@Trott
Copy link
Member

Trott commented May 29, 2017

@vladikoff
Copy link

Will it be possible to uplift this patch to node 4 and 6?

@sam-github
Copy link
Contributor

As a bug fix, it will land on 6.x after being released in 7.x for a while (usually a couple weeks, just to make sure there are no unexpected side effects).

4.x at this point requires an explicit request, we don't release unless we have to. It looks like not clearing the error stack can have wide-reaching consequences, but we prefer to not re-release 4.x unless necessary.

@vladikoff @rfk Do you have a need for this on 4.x? Some description of impact will make it much easier to make the case for it in 4.x

/cc @nodejs/lts

@rfk
Copy link
Contributor Author

rfk commented May 29, 2017

This bug affects our app because we happen to do both RSA signing and ECDH with user-submitted keys, so we recently deployed a workaround to production to guard against this being used as a DoS vector (i.e. deliberately submitting invalid ECDH keys to DoS the RSA signing functionality):

https://github.com/mozilla/fxa-auth-server/pull/1918/files#diff-539df6289b63785f3c1f4660c1db4107R174

I think this workaround means we can live without it on 4.x if we have to, although I'm not 100% confident, and I can't comment on how likely other apps are to be affected in a similar way. Having the fix on 4.x would obviously be preferable to this workaround from our point of view, but I understand that's not a trivial ask :-)

@sam-github
Copy link
Contributor

I think that's bad enough it is worth proposing for 4.x LTS

@indutny
Copy link
Member

indutny commented May 30, 2017

@sam-github I agree.

@jasnell
Copy link
Member

jasnell commented May 30, 2017

I agree that this is worth backporting to 4.x

@gibfahn
Copy link
Member

gibfahn commented May 30, 2017

+1 to backporting to v4.x.

The next question is how urgently it should happen I guess.

@sam-github
Copy link
Contributor

Is this the first thing that might conceivably be targettable at 4.x? Maybe the thing to do is open an issue in nodejs/LTS to update 4.x, and link any candidates. I haven't heard of any others, but I'm not the best source of news on that front.

@MylesBorins
Copy link
Contributor

MylesBorins commented May 31, 2017

there is a single commit sitting on v4.x-staging and one or two other random small ones that would be good to include. For the most part I believe we have been on top of backporting anything that would be good to have in a maintenance release... we could theoretically re audit against v6.x to double check

edit: worth mentioning there was a v4.x release last month... but if this warrants a new release it makes sense

@sam-github
Copy link
Contributor

Landed in f666533

@sam-github sam-github closed this May 31, 2017
sam-github pushed a commit that referenced this pull request May 31, 2017
Functions that call `ECDH::BufferToPoint` were not clearing the
error stack on failure, so an invalid key could leave leftover
error state and cause subsequent (unrelated) signing operations
to fail.

PR-URL: #13275
Reviewed-By: Fedor Indutny <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Sam Roberts <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@sam-github
Copy link
Contributor

It doesn't cherry-pick clean to 4.x or to 6.x, @rfk would you have the time to backport to 6.x? The process is to open a PR targeting v6.x-staging, title would be (backport 6.x) crypto: clear error stack in ECDH::BufferToPoint.

@sam-github
Copy link
Contributor

@nodejs/lts 4.8.3 was May 2nd, maybe we could do a 4.8.4 on July 2nd (aprox), to give time for any more patches to come in? 2 months seems a reasonable maintenance cadence.

@gibfahn
Copy link
Member

gibfahn commented May 31, 2017

@rfk
Copy link
Contributor Author

rfk commented May 31, 2017

It doesn't cherry-pick clean to 4.x or to 6.x, @rfk would you have the time to backport to 6.x?

Sure, I'll try to take a look later today.

@sam-github
Copy link
Contributor

@rfk Thank you.

jasnell pushed a commit that referenced this pull request Jun 5, 2017
Functions that call `ECDH::BufferToPoint` were not clearing the
error stack on failure, so an invalid key could leave leftover
error state and cause subsequent (unrelated) signing operations
to fail.

PR-URL: #13275
Reviewed-By: Fedor Indutny <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Sam Roberts <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@jasnell jasnell mentioned this pull request Jun 5, 2017
@gibfahn gibfahn mentioned this pull request Jun 15, 2017
3 tasks
gibfahn pushed a commit to gibfahn/node that referenced this pull request Jun 17, 2017
Functions that call `ECDH::BufferToPoint` were not clearing the
error stack on failure, so an invalid key could leave leftover
error state and cause subsequent (unrelated) signing operations
to fail.

PR-URL: nodejs#13275
Backport-PR-URL: nodejs#13397
Reviewed-By: Fedor Indutny <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Sam Roberts <[email protected]>
Reviewed-By: James M Snell <[email protected]>
gibfahn pushed a commit that referenced this pull request Jun 20, 2017
Functions that call `ECDH::BufferToPoint` were not clearing the
error stack on failure, so an invalid key could leave leftover
error state and cause subsequent (unrelated) signing operations
to fail.

PR-URL: #13275
Backport-PR-URL: #13397
Reviewed-By: Fedor Indutny <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Sam Roberts <[email protected]>
Reviewed-By: James M Snell <[email protected]>
MylesBorins pushed a commit that referenced this pull request Jul 10, 2017
Functions that call `ECDH::BufferToPoint` were not clearing the
error stack on failure, so an invalid key could leave leftover
error state and cause subsequent (unrelated) signing operations
to fail.

PR-URL: #13275
Backport-PR-URL: #13399
Reviewed-By: Fedor Indutny <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Sam Roberts <[email protected]>
Reviewed-By: James M Snell <[email protected]>
MylesBorins pushed a commit that referenced this pull request Jul 11, 2017
Functions that call `ECDH::BufferToPoint` were not clearing the
error stack on failure, so an invalid key could leave leftover
error state and cause subsequent (unrelated) signing operations
to fail.

PR-URL: #13275
Backport-PR-URL: #13397
Reviewed-By: Fedor Indutny <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Sam Roberts <[email protected]>
Reviewed-By: James M Snell <[email protected]>
MylesBorins pushed a commit that referenced this pull request Jul 11, 2017
Functions that call `ECDH::BufferToPoint` were not clearing the
error stack on failure, so an invalid key could leave leftover
error state and cause subsequent (unrelated) signing operations
to fail.

PR-URL: #13275
Backport-PR-URL: #13399
Reviewed-By: Fedor Indutny <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Sam Roberts <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@MylesBorins MylesBorins mentioned this pull request Jul 18, 2017
@MylesBorins MylesBorins mentioned this pull request Sep 20, 2017
MylesBorins pushed a commit that referenced this pull request Oct 25, 2017
Functions that call `ECDH::BufferToPoint` were not clearing the
error stack on failure, so an invalid key could leave leftover
error state and cause subsequent (unrelated) signing operations
to fail.

PR-URL: #13275
Backport-PR-URL: #13399
Reviewed-By: Fedor Indutny <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Sam Roberts <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@MylesBorins MylesBorins mentioned this pull request Oct 25, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c++ Issues and PRs that require attention from people who are familiar with C++. crypto Issues and PRs related to the crypto subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants