4
\$\begingroup\$

UPD: Haskell version, please check it too.

gameKey(long id) is authentication function of an MMO game server. When someone wants to play with friends, he sends request to server, server calls gameKey() and returns it as JSON to player via API. When a friend establishes connection, he sends the game id and his key, server calls gameKey(id), where id is user data, and then checks user's key.

Algorithm is:

  • use current time and internal counter to generate id if it's not provided, or use provided id
  • make fast crypto hash using Blake2 function of id and secret(Yes, I know about HMAC and using as key, but in this case it doesn't matter)
  • split 64-bit hash to four 16-bit values, one hash for one user
  • make JSON object from id and keys and return it

I want to make stable code: exploit-free, no memory leaks and high speed (for high load and DDOS prevention). I'm experimenting now, so the algorithm was first written in JavaScript, because I'm skilled in JS coding. Then I rewrote JS code in C++ to increase execution speed. You can see C++ and JS code below. The C++ code is 3x faster than JS: 34s vs 10s, per 1M keys

Please review the C++ code; I think it can be significantly improved (I'm bad in C++). Code compiles and works same as JS. Also please advise me, which programming language is better for this: JS, C++ , Haskell or Go?

Here's the C++ code; it requires libb2-dev and libjsoncpp-dev (run sudo apt install libb2-dev libjsoncpp-dev to install it on Ubuntu):

//"g++"  "main.cpp" -o "main" -lb2 -ljsoncpp
#include <sstream>
#include <string>
#include <iostream>
#include <ctime>
#include <blake2.h>//libb2-dev
#include <jsoncpp/json/json.h>//libjsoncpp-dev
#include <iomanip>

char TOPSECRET[] = "TOPSECRET";
long dateCounter = 0;

std::string gameKey(long id2=-1){

  long long id = std::time(0)*1000+dateCounter;
  if(id2!=-1){
    id = id2;
  }
  dateCounter++;

  std::stringstream inStream;
  inStream << id << "|keys|" << TOPSECRET;
  const std::string inString = inStream.str();
  const char* in = inString.c_str();

  size_t outlen=64;
  uint8_t* out = new uint8_t[outlen];
  char key[] = "";
  blake2b(out,in,&key,outlen,inString.length(),0);

  std::stringstream a,b,c,d;
  for(int i=0;i<16;i++){
    a << std::setfill('0') << std::setw(2) << std::hex << static_cast<unsigned>(out[i]);
  }
  for(int i=16;i<16*2;i++){
    b << std::setfill('0') << std::setw(2) << std::hex << static_cast<unsigned>(out[i]);
  }
  for(int i=16*2;i<16*3;i++){
    c << std::setfill('0') << std::setw(2) << std::hex << static_cast<unsigned>(out[i]);
  }
  for(int i=16*3;i<16*4;i++){
    d << std::setfill('0') << std::setw(2) << std::hex << static_cast<unsigned>(out[i]);
  }

  Json::Value obj(Json::objectValue);
  obj["id"] = id;

  Json::Value arr(Json::arrayValue);
  arr.append(a.str());
  arr.append(b.str());
  arr.append(c.str());
  arr.append(d.str());
  obj["keys"] = arr;

  Json::Writer* writer = new Json::FastWriter();
  std::string jsonOut = writer->write(obj);

  delete writer;
  //delete in;//comment=memory leak, uncomment=segfault when gameKey() is called more than once
  delete out;

  return jsonOut;
}

int main(){
  std::cout << gameKey();
  std::cout << gameKey();
}

Here's the node.js version; it needs blakejs (run npm i blakejs to install it):

const TOPSECRET = "TOPSECRET", blake = require("blakejs").blake2bHex;
let dateCounter = 0;

function gameKey(checkId){
  let id = checkId || (Date.now()/1000>>0)*1000+(dateCounter++%1000),
    keys = blake(`${id}|keys|${TOPSECRET}`).match(/.{32}/g);
  return {id, keys};
}
console.log(gameKey());
\$\endgroup\$
4
\$\begingroup\$

We have better random generators in C++ than std::time(0); if you're using the latter for its speed, it would really be worth a comment in the code to explain that you're willing to live with such a weak random-number source in exchange for the speed.

Note that embedding the secret in the code will make it visible to anyone who can obtain the source or the binary and read it. I'd recommend that you read it at runtime from a separate file - it's easier to correctly permission that one file rather than your source and all the build products.

For the case where the id is specified, there's no need to compute a time-based one just to throw it away immediately - I'd turn that conditional around:

std::string gameKey(long long id = -1)
{
  if (id == -1) {
    id = std::time(0) * 1000 + dateCounter++;
  }

Alternatively, write an overloaded function pair:

std::string gameKey()
{
   return gameKey(std::time(0) * 1000 + dateCounter++);
}

std::string gameKey(long long id)

A couple of misspellings: std::size_t and std::uint8_t (both missing namespace prefix in the code - it might work on your platform, but it's not portable).

I think that key can be a const char* or even a string literal:

blake2b(out, in, "", outlen, inString.length(), 0);

General: please give your code a bit more room to breathe - bunching up the source with no whitespace won't make it any more efficient.

Given that outlen is constant, we could declare it constexpr - then we wouldn't need to allocate out, but could declare it in automatic storage. There's certainly no need to allocate writer and have to delete it - just make it an automatic variable:

Json::Writer writer;
return writer.write(obj);
// look, no delete needed!

If you do need a variable outlen, at least use a smart pointer, so it's cleaned up for you.

Instead of writing to 4 buffers a, b, c and d, it might be simpler to write to a single buffer, and copy substrings from there (it would reduce those repeated loops with similar-but-different ranges).

| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ Thank you for detailed review. Why delete in causes segfault? How JSON coding can be improved? \$\endgroup\$ – Евгений Новиков Jul 18 '18 at 11:37
  • \$\begingroup\$ Not sure what you mean - there's no delete in that I see (and there shouldn't be, because we don't own in - it belongs to the string object and is just on loan). I don't know much about JSON, so can't advise there - perhaps another answerer will help? (Don't mark this accepted if you still want feedback from others). \$\endgroup\$ – Toby Speight Jul 18 '18 at 12:17
  • \$\begingroup\$ Ok, can it cause memory leak? Which objects we need to delete and which not? \$\endgroup\$ – Евгений Новиков Jul 18 '18 at 13:29
  • 2
    \$\begingroup\$ Simple rule - we delete everything we allocated with new (and delete[] everything we allocated with new[]). This is an decreasingly useful rule: in modern C++, there's rarely a need to use new, given that we prefer to create smart pointers instead. \$\endgroup\$ – Toby Speight Jul 18 '18 at 14:35
2
\$\begingroup\$

Compilation

Looking at your code, I would improve your compile command and explicitly target c++17, c++14 or c++11. This way you will have move semantics available.

Next to the standard, you also ignored optimizations completely, adding -O3 or -O2 would make the code even faster without much effort. Although in this case, the library will have most impact.

Some fine-tuning could be possible though might not be very noticeable.

Style

I'm sad to see that you use const in JavaScript and don't add const to TOPSECRET in C++.

-1 might be obvious to you, however, making a constant (for example INVALID_ID) out of it makes it clear to everyone.

Creating random numbers ain't cheap, so only do it when you need a new id. (And use if you need a good implementation) At the same time, it is more clear when a new random number gets created.

Creating some functions to split the code in more logical blocks would not hurt. Like: createHexString or saltInput.

Performance

StringStream ain't cheap, in this case with some int-to-string and regular string operations might be more performant. Reserving the string for the intended size could also make sense. On the other end, with it, the code is most readable.

Do we really need the setfill for each iteration? Compilers are smart in removing unneeded calls, however, in this case I don't get on it.

std::move might help in case of obj["keys"] = arr; if the library supports move semantics.

Don't allocate memory when not needed, see the answer of Toby for the details.

Misc

Finally, it's a pity that blake2b is a C library instead of C++. It requires you to work with character buffers and sizes instead of std:: string

Do you run the program 1M times or do you loop in that program?

You mentioned being exploit-free, however, I believe that is out of your control. To be sure, use a fuzzer to test the input.

Overall impressive code for being bad at C++.

| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ I tested loops, not running entire program. Because JS adds extra time to startup. I think optimisations can be avoided by reading id from cli with extra testing software, that inputs random valid ids \$\endgroup\$ – Евгений Новиков Jul 18 '18 at 18:29

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.