Fix NodeJS SecureSession handling nullptr #698
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Checking whether passed key is a private key is not enough since not all private keys are supported. For example, `secure_session_create()` may return NULL for RSA key. In this case, we get no exception on JS side but as soon as we try to connect to other peer (or accept connection) using created object, we get error. This commit makes sure we throw JS exception in case `secure_session_create()` returns NULL.
iamnotacake
added
the
W-JsThemis 🍭
ilammy
approved these changes
Aug 13, 2020
Comment on lines
+150
to
+153
| // secure_session_create() inside SecureSession::SecureSession() | ||
| // may fail, we need to catch this and throw exception to JS | ||
| ThrowParameterError("SecureSession", "unsupported private key"); | ||
| args.GetReturnValue().SetUndefined(); |
There was a problem hiding this comment.
Initially I wanted to note that there may be other reasons for a failure here, but most of them are actually checked and reported before this call. It can still fail due to allocation errors and such but that's rare so I think this message should be fine.
There was a problem hiding this comment.
Unfortunately, secure_session_create() only gives NULL or some valid pointer, and we can't get the actual error code in case of error.
Comment on lines
34
to
37
| bool isCreated() | ||
| { | ||
| return session_ != nullptr; | ||
| } |
There was a problem hiding this comment.
I guess this method can be private.
(I know, no one really cares in this case, but good habits are important.)
Make `isCreated()` a private method
3 tasks
iamnotacake
added a commit
that referenced
this pull request
Aug 19, 2020
* Add Go tests * Add JS tests * Add Python tests, fix Python wrapper * Add Ruby tests, fix Ruby wrapper * Add C++ tests, fix typos * Check Secure Session constuction failure in ObjCThemis Verify that Secure Session has been constructed successfully. This should catch RSA keys being passed to the initializer earlier. * Update changelog, mention fixes of NodeJS (#698), Python, Ruby, ObjC, Swift wrappers * Update .gitignore by adding few things like Ruby `.gem` packages, Python package metadata, another Rust `target` directory The 'tests' above are: * Whether Secure Session constructor accepts EC private key * Whether Secure Session constructor rejects EC public and RSA private and RSA public keys The fixes mentioned above were wrong/missing checks whether `secure_session_create()` returned NULL. Python and Ruby were doing it wrong and didn't throw the exception while ObjC was completely missing the check. Co-authored-by: Alexei Lozovsky <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Checking whether passed key is a private key is not enough since not all
private keys are supported. For example,
secure_session_create()mayreturn NULL for RSA key. In this case, we get no exception on JS side
but as soon as we try to connect to other peer (or accept connection)
using created object, we get error. This commit makes sure we throw JS
exception in case
secure_session_create()returns NULL.Not sure whether this is the best way to do the check but AFAIK using C++ exceptions
inside NodeJS bindings may not be a good idea. Other idea was to throw something from
SecureSession::SecureSession()and then putSecureSession* obj = new SecureSession(...);into
try {}block. Also, why throw the exception if it will be immediately checked afterwards,and it does not contain any useful information, just failed or succeed (no exception).
Checklist
Change is covered by automated testsThis change (obviously) does not break existing tests.
However, tests like "make sure SecureSession does now allow RSA private keys (as well as any public)"
will be done as part of different PR. And it won't be NodeJS wrapper only.