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.

Sign up for GitHub

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

Jump to bottom

Symmetric keygen: JsThemis #562

Merged
merged 5 commits into from
Dec 11, 2019
Merged

Symmetric keygen: JsThemis #562

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
33115ec
Symmetric keygen: JsThemis
ilammy Dec 3, 2019
1341dc2
Stricter length tests
ilammy Dec 6, 2019
919f283
More empty buffer checks
ilammy Dec 6, 2019
a248531
More instance tests
ilammy Dec 6, 2019
364110f
fixup! More instance tests
ilammy Dec 6, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,10 @@ _Code:_
- Fixed a NullPointerException bug in `SecureSocket` initialisation ([#557](https://github.com/cossacklabs/themis/pull/557)).
- Some Themis exceptions have been converted from checked `Exception` to _unchecked_ `RuntimeException`, relaxing requirements for `throws` specifiers ([#563](https://github.com/cossacklabs/themis/pull/563)).

- **Node.js**

- New class `SymmetricKey` can be used to generate symmetric keys for Secure Cell ([#562](https://github.com/cossacklabs/themis/pull/562)).

- **Python**

- Fixed compatibility issues on 32-bit platforms ([#555](https://github.com/cossacklabs/themis/pull/555)).
Expand Down
1 change: 1 addition & 0 deletions src/wrappers/themis/jsthemis/addon.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@ void InitAll(v8::Local<v8::Object> exports)
jsthemis::Errors::Init(exports);
jsthemis::SecureMessage::Init(exports);
jsthemis::KeyPair::Init(exports);
jsthemis::SymmetricKey::Init(exports);
jsthemis::SecureSession::Init(exports);
jsthemis::SecureCellSeal::Init(exports);
jsthemis::SecureCellContextImprint::Init(exports);
Expand Down
96 changes: 96 additions & 0 deletions src/wrappers/themis/jsthemis/secure_keygen.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -173,4 +173,100 @@ bool IsPublicKey(const std::vector<uint8_t>& key)
return false;
}

Nan::Persistent<v8::Function> SymmetricKey::constructor;

void SymmetricKey::Init(v8::Local<v8::Object> exports)
{
v8::Local<v8::String> className = Nan::New("SymmetricKey").ToLocalChecked();

// Prepare constructor template
v8::Local<v8::FunctionTemplate> thisTemplate = Nan::New<v8::FunctionTemplate>(SymmetricKey::New);
thisTemplate->SetClassName(className);

// Export constructor
v8::Local<v8::Function> function = Nan::GetFunction(thisTemplate).ToLocalChecked();
constructor.Reset(function);
Nan::Set(exports, className, function);
}

void SymmetricKey::New(const Nan::FunctionCallbackInfo<v8::Value>& args)
{
// If not invoked as "new themis.SymmetricKey(...)" then reinvoke.
if (!args.IsConstructCall()) {
// We support at most one argument, pass it and ignore others.
v8::Local<v8::Value> argv[1] = {args[0]};
v8::Local<v8::Function> cons = Nan::New<v8::Function>(constructor);
args.GetReturnValue().Set(Nan::NewInstance(cons, args.Length(), argv).ToLocalChecked());
return;
}

// If invoked as "new themis.SymmetricKey()" then generate a new key.
if (args.Length() == 0) {
std::vector<uint8_t> buffer;

size_t length = 0;
themis_status_t status = themis_gen_sym_key(NULL, &length);
if (status != THEMIS_BUFFER_TOO_SMALL) {
ThrowError("Themis SymmetricKey", status);
args.GetReturnValue().SetUndefined();
return;
}

buffer.resize(length);
status = themis_gen_sym_key(&buffer.front(), &length);
if (status != THEMIS_SUCCESS) {
ThrowError("Themis SymmetricKey", status);
args.GetReturnValue().SetUndefined();
return;
}

args.GetReturnValue().Set(CopyIntoBuffer(buffer));
return;
}

// If invoked as "new themis.SymmetricKey(value)" then value must be
// a byte buffer that we copy.
v8::Local<v8::Value> value = args[0];
if (!value->IsUint8Array()) {
ThrowParameterError("Themis SymmetricKey",
"key is not a byte buffer (use Buffer or Uint8Array)");
args.GetReturnValue().SetUndefined();
return;
}
if (node::Buffer::Length(value) == 0) {
ThrowParameterError("Themis SymmetricKey", "key is empty");
args.GetReturnValue().SetUndefined();
return;
}

args.GetReturnValue().Set(CopyIntoBuffer(value));
Copy link
Collaborator

Choose a reason for hiding this comment

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

if user pass own buffer, why we don't reuse it and return new allocated buffer?
CopyIntoBuffer use nan::CopyBuffer instead nan::NewBuffer which will reuse passed array

Copy link
Collaborator

Choose a reason for hiding this comment

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

oh, I re-read comments and understand that we just copy passed buffer, not generate new key... but for what we do this or why it will useful for user? is I understand correctly, that when user pass some Buffer to constructor themis.SymmetricKey(someBuffer) we return node::Bufer object with same values?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

but for what we do this or why it will useful for user?

Well, we have to copy the buffer so that changes in the original buffer do not affect the key.

As for why this API exists at all, my original idea was that new themis.SymmetricKey(...) would return an instance of SymmetricKey — a type that inherits from Buffer, but is not a Buffer. Then it will be possible to restrict Secure Cell interfaces to work only with SymmetricKey instances which are guaranteed to contain only valid keys. In this case the users will need an ability to convert an arbitrary Buffer into SymmetricKey with JsThemis validating the key.

Currently new themis.SymmetricKey() returns instances of buffer, but even if that’s not fixed by the next release I would like to keep this API so that we teach the users to use it instead of using Buffers directly.

Copy link
Collaborator

Choose a reason for hiding this comment

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

as I see, nan and nodejs buffer api provide two ways to create objects. New created with copy of values nan::FromBuffer and second way to reuse user's object new nan::NewBuffer. What about to be appropriate with their api and provide two ways too?

Copy link
Collaborator Author

@ilammy ilammy Dec 6, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

Nan::NewBuffer() transfers the ownership over a char* buffer to Node.js Buffer object. That’s not what we want here since the original byte buffer is still owned by the original Buffer object. It’s a utility method to transfer buffers created by C++ code into JavaScript without making an unnecessary copy.

When the user does new SymmetricKey(someBuffer) it does not transfer someBuffer into SymmetricKey. Unfortunately, JavaScript is not Rust and even if we did reuse an existing Buffer as SymmetricKey, this will not prevent the user from using someBuffer again after the call. For example, they could modify the buffer and this will modify the key. I don’t think that this spooky action at the distance is an expected behavior so it’s better to copy the buffer with Nan::CopyBuffer() and avoid such issues.

Copy link
Collaborator

Choose a reason for hiding this comment

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

It's okay if they know, that they transfer ownership. in such case they use new SymmetricKey(someBuffer) just to cast their buffer to correct type to use with themis in the future. If our function doesn't modify this buffer, it's okay if user use previously created object to re-use if he know, that don't need it in a future. For example (js pseudocode):

var buffer = Buffer.alloc(32)
var keys = ['32 length key1', '32 length key2']
var data = ['data1', 'data2']
var encrypted = []
for (i:=0; i<data.length; i++){
  var key = new themis.SymmetricKey(keys[i]);
  encryptedData = new themis.SecureCell(key).encrypt(data[i]);
  encrypted.push(encryptedData);
}

here is user reuse same buffer and use it just to cast his keys to correct type and use with themis SecureCell api.

But if we can't do like that, so we only can leave as is...

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The above code will work just fine. That's what a sane developer (i.e., one thinking like me) would probably write if they wanted to encrypt data with different keys.

The current copying implementation makes the following usage impossible:

var buffer = Buffer.alloc(32)
var key = new themis.SymmetricKey(buffer) // note that it's out of the loop
var cell = new themis.SecureCell(key)

var data = ['data1', 'data2']
var encrypted = []
for (var i = 0; i < data.length; i++) {
    getSomeKeyDataInto(buffer)
    encrypted.push(cell.encrypt(data[i]))
}

While it definitely has a rationale of a sort, I'd consider it premature optimization and, honestly, a sleeping footgun. It looks like the same cell is used for encryption while in fact the key is different.

Now imagine if the Buffer is actually a reference that's kept in a field or some object. And that object caches buffer instances so the same buffer may be reused later for some completely unrelated data. Even if you don't abuse the buffer like the above, it may still be accidentally changed.

That's why I think that copying the buffer is the most reasonable choice here.

return;
}

// TODO: return properly inherited instances of SymmetricKey
//
// Currently "new themis.SymmetricKey()" produces instances of Buffer.
// This works in practice (because JavaScript), but it may be unexpected
// as "key instanceof themis.SymmetricKey" returns false.
//
// Unfortunately, V8 does not make JavaScript prototype inheritance easier
// and I was not able to implement it correctly. It would be nice is someone
// made SymmetricKey constructor return SymmetricKey instances that inherit
// from Buffer and get all Node.js utities for free.

v8::Local<v8::Object> SymmetricKey::CopyIntoBuffer(const std::vector<uint8_t>& buffer)
{
const char* data = NULL;
if (!buffer.empty()) {
data = reinterpret_cast<const char*>(&buffer.front());
}
uint32_t length = buffer.size();
return Nan::CopyBuffer(data, length).ToLocalChecked();
}

v8::Local<v8::Object> SymmetricKey::CopyIntoBuffer(v8::Local<v8::Value> buffer)
{
return Nan::CopyBuffer(node::Buffer::Data(buffer), node::Buffer::Length(buffer)).ToLocalChecked();
}

} // namespace jsthemis
14 changes: 14 additions & 0 deletions src/wrappers/themis/jsthemis/secure_keygen.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,20 @@ bool IsValidKey(const std::vector<uint8_t>& key);
bool IsPrivateKey(const std::vector<uint8_t>& key);
bool IsPublicKey(const std::vector<uint8_t>& key);

class SymmetricKey : public Nan::ObjectWrap
{
public:
static void Init(v8::Local<v8::Object> exports);

private:
static void New(const Nan::FunctionCallbackInfo<v8::Value>& args);

static v8::Local<v8::Object> CopyIntoBuffer(const std::vector<uint8_t>& buffer);
static v8::Local<v8::Object> CopyIntoBuffer(v8::Local<v8::Value> buffer);

static Nan::Persistent<v8::Function> constructor;
};

} // namespace jsthemis

#endif /* JSTHEMIS_KEY_PAIR_HPP_ */
28 changes: 25 additions & 3 deletions tests/jsthemis/test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ describe("jsthemis", function(){
decrypter = new addon.SecureMessage(peer_keypair.private(), keypair.public());
intruder_decrypter = new addon.SecureMessage(intruder_keypair.private(), keypair.public());
message = new Buffer("Test Message");
it("encrypt/decrypt", function(){
it("encrypt/decrypt", function(){
encrypted_message = encrypter.encrypt(message);
assert.equal(message.toString(), decrypter.decrypt(encrypted_message).toString());
assert.throws(function(){intruder_decrypter.decrypt(encrypted_message);}, expect_code(addon.FAIL));
Expand Down Expand Up @@ -86,7 +86,7 @@ describe("jsthemis", function(){
server_keypair = new addon.KeyPair();
client_id = new Buffer("client");
client_keypair = new addon.KeyPair();

server_session = new addon.SecureSession(server_id, server_keypair.private(), function(id){
if(id.toString()=="server")
return server_keypair.public();
Expand Down Expand Up @@ -175,6 +175,28 @@ describe("jsthemis", function(){

describe("jsthemis", function(){
describe("secure cell", function(){
describe("key generation", function(){
const defaultLength = 32
it("generates new key buffer", function(){
var masterKey = new addon.SymmetricKey()
assert.equal(masterKey.length, defaultLength)
Copy link
Collaborator

Choose a reason for hiding this comment

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

actually here not checked that generated new key buffer. we should compare references/values to be sure, not that same length was used.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Mmm... I’d say this is a bit excessive. But sure, we can add

var key1 = new addon.SymmetricKey()
var key2 = new addon.SymmetricKey()
assert.notDeepEqual(key1, key2) // checks that content is not the same
assert.notEqual(key1, key2) // checks that objects are distinct

Though I doubt that this test will prevent any coding mistakes.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I like to add it because we should be sure that our new layer of js code correctly uses themis core. If it was simple proxying calls like in c++/golang then it will be really overhead. But in wrappers like jsthemis/phpthemis where we deal with language wrapper API we should do more checks IMHO because we have more places for mistakes.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Okay, now I get your point. That’s a pretty valid concern. Thank you for the explanation.

})
it("is able to restore SymmetricKey from bytes", function(){
var bytes = Buffer.from("MDRwUzB0NG1aN2pvTEEwdVljRFJ5", "base64")
var masterKey = new addon.SymmetricKey(bytes)
assert.equal(masterKey.length, bytes.length)
})
it("throws on empty buffer", function(){
assert.throws(() => new addon.SymmetricKey(Buffer.from("")),
vixentael marked this conversation as resolved.
Show resolved Hide resolved
expect_code(addon.INVALID_PARAMETER))
assert.throws(() => new addon.SymmetricKey(""),
expect_code(addon.INVALID_PARAMETER))
assert.throws(() => new addon.SymmetricKey(null),
expect_code(addon.INVALID_PARAMETER))
assert.throws(() => new addon.SymmetricKey(undefined),
expect_code(addon.INVALID_PARAMETER))
})
})
message=new Buffer("This is test message");
password=new Buffer("This is test password");
context=new Buffer("This is test context");
Expand Down Expand Up @@ -217,7 +239,7 @@ describe("jsthemis", function(){
context_imprint_intruder_decrypter = new addon.SecureCellContextImprint(new Buffer("This is test password1"));
assert.throws(function(){new addon.SecureCellContextImprint(empty_message)});
context_imprint_enc_data = context_imprint_encrypter.encrypt(message, context);
assert.equal(message.length, context_imprint_enc_data.length);
assert.equal(message.length, context_imprint_enc_data.length);
context_imprint_dec_data = context_imprint_decrypter.decrypt(context_imprint_enc_data, context);
assert.equal(message.toString(), context_imprint_dec_data.toString());
context_imprint_dec_data = context_imprint_intruder_decrypter.decrypt(context_imprint_enc_data, context);
Expand Down