Secure Session: add checks to pubkey callback

It turned out that public key callback did not have enough checks and could crash Node process if the user returns unexpected value from it. Add type checks for result: an array is okay, null and undefined are explicit "not found" values, anything else is also considered "not found" but with a different error code (the exact value does not matter at the moment). Also add a length check to avoid buffer overflow when doing memcpy().
cossacklabs · Feb 19, 2019 · 6a6b540 · 6a6b540
1 parent 593dedb
commit 6a6b540
Show file tree

Hide file tree

Showing 2 changed files with 50 additions and 2 deletions.
diff --git a/src/wrappers/themis/jsthemis/secure_session.cpp b/src/wrappers/themis/jsthemis/secure_session.cpp
@@ -30,8 +30,18 @@ namespace jsthemis {
       return THEMIS_BUFFER_TOO_SMALL;
     v8::Local<v8::Value> argv[1] = { Nan::CopyBuffer((char*)id, id_length).ToLocalChecked() };
     v8::Local<v8::Value> a = Nan::Call(((SecureSession*)(user_data))->id_to_pub_key_callback_, 1, argv).ToLocalChecked();
-    std::memcpy(key_buffer, (const uint8_t*)(node::Buffer::Data(a.As<v8::Object>())), node::Buffer::Length(a.As<v8::Object>()));
-    return THEMIS_SUCCESS;
+    if(a->IsUint8Array()){
+      v8::Local<v8::Object> buffer = a.As<v8::Object>();
+      if(key_buffer_length < node::Buffer::Length(buffer)){
+        return THEMIS_BUFFER_TOO_SMALL;
+      }
+      std::memcpy(key_buffer, (const uint8_t*)(node::Buffer::Data(buffer)), node::Buffer::Length(buffer));
+      return THEMIS_SUCCESS;
+    } else if(a->IsNull() || a->IsUndefined()) {
+      return THEMIS_FAIL;
+    } else {
+      return THEMIS_INVALID_PARAMETER;
+    }
   }
 
   SecureSession::SecureSession(const std::vector<uint8_t>& id, const std::vector<uint8_t>& private_key, v8::Local<v8::Function> get_pub_by_id_callback):

diff --git a/tests/jsthemis/test.js b/tests/jsthemis/test.js
@@ -95,6 +95,44 @@ describe("jsthemis", function(){
 	    server_session = new addon.SecureSession(valid_id, keypair.private(), function(){return null});
 	    assert.throws(function(){server_session.unwrap(empty_id)}, expect_code(addon.INVALID_PARAMETER));
 	})
+        it("callback behavior", function(){
+            message = new Buffer("This is test message")
+            server_id = new Buffer("server")
+            server_keypair = new addon.KeyPair()
+            client_id = new Buffer("client")
+            client_keypair = new addon.KeyPair()
+            broken_id = new Buffer("broken")
+            missing_id = new Buffer("missing")
+            unknown_id = new Buffer("unknown")
+
+            new_server_session = function() {
+                return new addon.SecureSession(server_id, server_keypair.private(), function(id){
+                    if(id.toString() == client_id.toString())
+                        return client_keypair.public()
+                    else if(id.toString() == missing_id.toString())
+                        return null
+                    else if(id.toString() == broken_id.toString())
+                        return 42
+                })
+            }
+
+            server_callback = function(id){
+                if(id.toString() == server_id.toString())
+                    return server_keypair.public()
+            }
+            client_sessions = []
+            client_sessions.push(new addon.SecureSession(broken_id, client_keypair.private(), server_callback))
+            client_sessions.push(new addon.SecureSession(missing_id, client_keypair.private(), server_callback))
+            client_sessions.push(new addon.SecureSession(unknown_id, client_keypair.private(), server_callback))
+
+            client_sessions.forEach(function(client_session){
+                var server_session = new_server_session()
+                var data
+
+                data = client_session.connectRequest();
+                assert.throws(function(){server_session.unwrap(data)}, expect_code(addon.SSESSION_GET_PUB_FOR_ID_CALLBACK_ERROR));
+            })
+        })
     })
 })