Optimize Secure Message signature length

[ilammy]

GoThemis ignores actual Secure Message signature length returned by Themis, and instead returns the originally allocated buffer as is, which might contain some extra unused bytes at the end.

While not fatal—Themis should still recognize both padded and unpadded signatures—this behavior differs from other Themis wrappers.

Fix the issue in Secure Message processing which results in overlong signatures. Also, add tests to GoThemis specifically to check that both overlong and proper messages are accepted.

_{This might be considered a breaking change for “smart” users who relied on Secure Message signatures having a particular length, and for them to have a fixed length in GoThemis. Shame on them. I am not aware of sane use cases that involve this assumption. If you are one of those, pad Secure Message signature to len(message) + 84, and pray that Themis never changes the ageing P-256 elliptic curve to something different.}

Checklist

• Change is covered by automated tests
• Benchmark results are attached (should be negligible)
• The coding guidelines are followed
• Changelog is updated

[ilammy]

Added the same fix and test to ThemisPP.

The test data could look prettier, but we have to support C++03 and clang-fmt holds a strong opinion on how you should write your code with giant arrays.

Based on my review, JavaThemis is also affected by the same “bug” with signature lengths. I guess it wouldn't hurt to add tests there as well. However, the fix is not that straightforward as here, because you can't resize arrays in JVM, hence it would not be O(1), like for Go and C++.

[ilammy]

(Also, there are other cryptosystems where GoThemis, ThemisPP, JavaThemis don't respect actual buffer lengths returned on the second call. I wonder if those should be fixed. At all, that is, not in this PR. This PR is about Secure Message specifically as it's a known quirk. But I don't know if the same thing has funny effects on, say, Secure Session protocol.)

[vixentael]

if the same thing has funny effects on, say, Secure Session protocol

well..

All checks have passed
11 successful checks

let's hope it doesn't :)

[vixentael]

nice optimization!

I remember (found in the internal issue tracker), that @Lagovas has similar idea before. I'd like to wait until @Lagovas takes a look on this PR

[ilammy]

@vixentael,

if the same thing has funny effects on, say, Secure Session protocol

well..

All checks have passed
11 successful checks

let's hope it doesn't :)

I did not mean right now – of course it works right now, just like Secure Message worked, for a certain definition thereof.

I meant more like, what if this causes some subtle incompatibilities between platforms because of this. I don't really remember any integration tests for Secure Session, for example, which ensure that Secure Session in Python can talk to Secure Session in Go (or hard mode: the same thing but Themis versions are also different).

But knowing all the answers wouldn't be fun, right?

I remember (found in the internal issue tracker), that @Lagovas has similar idea before.

Yeah, I remember something like that. Though, I also remember some wariness to make this change as it might break something Acra because Acra does depend on Themis implementation details in some cases. Though, as I view it, AcraStructs should not be affected by this specifically because they are using Secure Message in encryption mode, not for signatures, and they do include lengths. But again, I don't really remember what that ticket was all about so please take care to double-check that this is not a breaking change.

[ilammy]

Proposed changes to JavaThemis look like this: ilammy@c40ebb9

• Allocate a temporary buffer in native code first, pass that to Themis (instead of allocating a Java array and passing a reference to its bytes)
• After Themis is done and successful, allocate a Java array and copy results there
• Use GetPrimitiveArrayCritical and ReleasePrimitiveArrayCritical to hint JVM that we'll be quick

This is a bit of a pessimization in a happy case: JVM does no copies and Themis is able to write its output directly into heap memory. With the fix JNI code has to allocate a buffer on native heap first, then allocate JVM array of the correct size and copy data there. Not quite a fairyland that it was before.

It should not be a massive performance hit, but it's definitely a hit.

Moreover, strictly speaking, the code should behave like that everywhere, not only in Secure Message. Currently all other cryptosystems in Java ignore the length returned by the second call to Themis function. It's just that for them the initial assessment is accurate so we don't end up with a couple of extra bytes at the end.

[Lagovas]

yeah, acra doesn't depend on signatures, and btw it has pinned version of gothemis, so it should not break anything for now. Only if we will update gothemis version. But it should not be so