Malformed password argument leads to null pointer deference & segmentation fault

[anglinb]

👋 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 a String type) to char * rely on v8::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 a char *.

According to the v8::String::Utf8Value documentation:

If conversion to a string fails (e.g. due to an exception in the toString() method of the object) then the length() method returns 0 and the * operator returns NULL.

However, node-keytar never checks to make sure this conversion was successful so when SetPasswordWorker's constructor is called, it calls std::string's constructor with a null pointer:

password(password) 

// This ^^^ is a weird C++ism but it means this:

this->password = std::string(password)

Link to the code ^

As explained in documentation C++

If s is a null pointer, if n == npos, or if the range specified by [first,last) is not valid, it causes undefined behavior

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

[shiftkey]

@anglinb thanks for digging into this! I ended up finding a simpler check that nan provides, and throwing a type error with more helpful context:

if (!info[0]->IsString()) {
    Nan::ThrowTypeError("Parameter 'service' must be a string");
    return;
}

The original test suggested "gracefully handling" this error, but I think reporting a TypeError here feels more predictable.