This repository has been archived by the owner on Dec 15, 2022. It is now read-only.
Malformed password argument leads to null pointer deference & segmentation fault #71
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.
馃憢 Was taking a look at the project w/ @philipturnbull and we came across a small issue in which a malformed password argument (specially one whose
toString()
method throws an exception) leads to a null pointer deference and segmentation fault.This does not pose a security risk but patching up this functionality could make
node-keytar
more robust 馃憤Explanation
tl;dr An error in
toString()
leads to node-keytar calling an std::string constructor w/ a null pointer which leads to undefined behavior (and seg faults for me).All methods in
node-keytar
that cast a Javascript object (usually aString
type) tochar *
rely onv8::String::Utf8Value
which is a generally safe method to call. In the code,v8::String::Utf8Value
is called with the*
operator:*v8::String::Utf8Value(info[0]),
which returns achar *
.According to the
v8::String::Utf8Value
documentation:However,
node-keytar
never checks to make sure this conversion was successful so whenSetPasswordWorker
's constructor is called, it callsstd::string
's constructor with a null pointer:Link to the code ^
As explained in documentation C++
In my build, I'm seeing the process seg fault.
Impact
There is very minimal impact unless a non-string type is being passed to
node-keytar
. Even so, this would just crash the process. I'm opening this issue to document this behavior and to show how unexpected javascript objects can cause problems in native modules in what seems like a sane implementation.Remediation
To fix this issue, the C++ code should check that the result of calling
v8::String::Utf8Value
is non-null before passing on the result./cc @BinaryMuse @philipturnbull @gregose