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.

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

[Feature Request] Signed Bytecode Loading #2048

Closed
SoniEx2 opened this issue Sep 2, 2016 · 16 comments
Closed

[Feature Request] Signed Bytecode Loading #2048

SoniEx2 opened this issue Sep 2, 2016 · 16 comments

Comments

@SoniEx2
Copy link
Contributor

SoniEx2 commented Sep 2, 2016

string.dump() should be replaced with a function that signs the resulting bytecode. load should verify bytecode signatures.

The signature should be generated server-side. There's no need to have more than one signature per server. The whole signature process should be done in Java/Scala so as to not leak the key. The key should be loaded into a byte[] every time it is to be used, and the byte[] should be cleared (set to 0) every time after use, so as to not leak the key. This byte[] should be global and synchronized, to reduce the risk of leaking the key.

This lets you get all the benefits of bytecode (smaller size, faster loading, etc) without any of the drawbacks (because only bytecode produced by the server can be loaded).

@SoniEx2
Copy link
Contributor Author

SoniEx2 commented Sep 3, 2016

Some additional notes: Signing may be overkill. HMAC might also work just fine. Can also use Scrypt or something, but that might be about as overkill as Ed25519 signatures...

Not sure if there is a concept of "overkill crypto" tho. You'd think stronger crypto is always better...

@magik6k
Copy link
Contributor

magik6k commented Sep 3, 2016

Just encrypting Sha hash with AES would be enough. As for security considerations I'm not actually sure, but just appending signature to the blob looks good enough to me.

@SoniEx2
Copy link
Contributor Author

SoniEx2 commented Sep 3, 2016

@magik6k HMAC is simpler as it only requires a hash function.

Also who even rolls their own crypto (MAC in this case)? D:

@SoniEx2 SoniEx2 changed the title Signed Bytecode Loading [Feature Request] Signed Bytecode Loading Sep 22, 2016
@SoniEx2
Copy link
Contributor Author

SoniEx2 commented Sep 22, 2016

Am I the only one who cares about this?

@payonel
Copy link
Member

payonel commented Sep 22, 2016

Actually I like this idea, no need to bump, but I'll add feature suggested, and i'll let sangar decide if it is feature accepted/open-for-adoption

@fnuecke
Copy link
Member

fnuecke commented Oct 15, 2016

I suppose that could work, in principle. Is it worth it though? What are the actual, practical use-cases?

@SoniEx2
Copy link
Contributor Author

SoniEx2 commented Oct 15, 2016

Saves space, loads faster (might wanna compile the OpenOS .lua files when installing - and/or add a cache to it, altho that'd be much more complex), etc.

The EEPROMs would get to fit a bit more Lua code than they currently do, I think. Assuming they still work the same way - I haven't played this mod in ages...

@payonel
Copy link
Member

payonel commented Oct 15, 2016

I'd use it for swap space memory

@SoniEx2
Copy link
Contributor Author

SoniEx2 commented Oct 16, 2016

Ok so, assuming HMAC:

If string: (use cases: stored procedures or something)

  1. Check if the string describes a binary chunk.
  2. Split the string into "data" and "hmac".
  3. Compute the hmac over "data".
  4. Compare the computed hmac with the hmac from step 1.
  5. If they match, load "data", otherwise fail.

If stream (function): (use cases: loading from file)

  1. Create a ring buffer, the size of the HMAC.
  2. Call the function, loading the bytes into the buffer. (this is also where you check if the return value describes a binary chunk, if it doesn't you skip all the next steps.)
  3. If the function returns an end of chunk (nil, empty string, no value), compare the calculated HMAC with the provided HMAC, and continue/error as needed.
  4. As the bytes come out of the buffer, use them to update the HMAC, and pass them on to the original load() function.

If mode is just "b" or just "t" you can skip the type checks (remember to pass the mode on to the original load() function!), but if mode is "bt" you need to do the checks.

@LordFokas
Copy link
Contributor

Why would you need it to be signed though? To prevent code injection?

@SoniEx2
Copy link
Contributor Author

SoniEx2 commented Oct 16, 2016

@payonel
Copy link
Member

payonel commented Dec 4, 2017

I investigated this feature in my emulator and found that using string.dump for memory swap would actually be an enormous undertaking and we'd have to parse the byte ourselves in some cases (for example serializing upvalues) So I don't find this worth it to support string.dump

@payonel payonel closed this as completed Dec 4, 2017
@SoniEx2
Copy link
Contributor Author

SoniEx2 commented Dec 4, 2017

swap? nah. this is just for stripping debug symbols and optionally precompiling each .lua file in OpenOS during install (so it can be installed on a machine with very little RAM and disk space).

y'know what would work for runtime RAM management? shrinking buffers/disk cache (buffer.lua) if the RAM is low.

@skyem123
Copy link
Contributor

skyem123 commented Dec 4, 2017

Why not make it just check against a list of SHA hashes? The list could be provided by OC and plugin mods as they are already trusted to run code.
While it'd be okay to allow dumping the string and adding that to a list of hashes in the save file, as it is kinda already loading right into memory from there with the whole persistence system, it's pointless because any modifications you'd want to make would fail the hash.

@SoniEx2
Copy link
Contributor Author

SoniEx2 commented Dec 5, 2017

The point is not to make modifications. The point is to eliminate the "Lua parser" step (saves CPU time) and strip debug information (saves RAM and disk space). The former is done by simply loading bytecode. The latter is done with string.dump(f, true) - this is built-in, at least in Lua 5.3.

@skyem123
Copy link
Contributor

skyem123 commented Dec 5, 2017

Alright, in that case, then it could work and if implemented correctly should no worse in security than the current system of saving state.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

6 participants