Skip to content

Conversation

sjaeckel
Copy link
Member

@sjaeckel sjaeckel commented Oct 3, 2025

I've reordered and refactored some of the commits of #697 in order to limit its scope and simplify the merge and a potential faster release of 2.0

With these changes merged I currently don't see any planned ABI or API breakage (besides #515, but that's an entirely different discussion).

This should be the last PR with intentional changes before a v2.0.0-rc1, c.f. #568

@sjaeckel sjaeckel added this to the next milestone Oct 3, 2025
@sjaeckel sjaeckel requested review from karel-m and levitte October 3, 2025 09:22
@karel-m
Copy link
Member

karel-m commented Oct 3, 2025

I have just quickly changed the libtomcrypt in my CryptX perl bindings for the branch "some-improvements" from this PR.

It failed to build (which is probably expected) https://github.com/DCIT/perl-CryptX/actions/runs/18219248768/job/51875345151

I will try to adopt CryptX to the new changes (after that we will see whether CryptX test suite reveals something).

@sjaeckel sjaeckel force-pushed the some-improvements branch 2 times, most recently from 24f8a2f to 787e5c1 Compare October 3, 2025 13:39
@sjaeckel
Copy link
Member Author

sjaeckel commented Oct 3, 2025

I have just quickly changed the libtomcrypt in my CryptX perl bindings for the branch "some-improvements" from this PR.

Cool, thanks!

It failed to build (which is probably expected) https://github.com/DCIT/perl-CryptX/actions/runs/18219248768/job/51875345151

If I'm not mistaken there are two errors

[148] ff. is already caused by #524

[187] ff. is indeed caused by the API change of rsa_verify_hash_ex() in this PR

Both are expected, so thanks already in advance for fixing this in CryptX and testing those changes ☺️

[148] https://github.com/DCIT/perl-CryptX/actions/runs/18219248768/job/51875345151#step:7:149
[187] https://github.com/DCIT/perl-CryptX/actions/runs/18219248768/job/51875345151#step:7:188

@karel-m
Copy link
Member

karel-m commented Oct 5, 2025

I have updated my perl bindings and it seems to work fine.

Just one minor suggestion: please add @param mgf_hash_idx ..... to description of rsa_sign_hash_ex + rsa_verify_hash_ex

@sjaeckel sjaeckel force-pushed the some-improvements branch 2 times, most recently from 5b5fec8 to c760347 Compare October 16, 2025 08:31
@sjaeckel sjaeckel linked an issue Oct 16, 2025 that may be closed by this pull request
Signed-off-by: Steffen Jaeckel <[email protected]>
To be able to do a bit more, add an optional handler callback function.
Additional to that, also make it possible to mark elements as optional.

Signed-off-by: Steffen Jaeckel <[email protected]>
(and you should do that too)

Signed-off-by: Steffen Jaeckel <[email protected]>
Signed-off-by: Steffen Jaeckel <[email protected]>
Update PKCS#1-PSS and RSA APIs that allow passing a separate hash index for
the MGF1 hash.

Signed-off-by: Steffen Jaeckel <[email protected]>
Signed-off-by: Steffen Jaeckel <[email protected]>
Slightly minimize both space and time when importing a
SubjectPublicKeyInfo. Time for ECC keys stays the same.

Those tests were done with X.509 support already available, but later these
commits were split up to be independent of the X.509 feature.

Running the entire set of pem files through `x509_verify` via [0]
resp. the timing app via [1] resulted in the following data:

Before this patch:

[0]
```
==1031519== HEAP SUMMARY:
==1031519==     in use at exit: 0 bytes in 0 blocks
==1031519==   total heap usage: 424,057 allocs, 424,057 frees, 73,527,730 bytes allocated
```

[1]
```
x509 cert-rsa-pss.pem    :     50021 cycles
x509 LTC_CA.pem          :     10335 cycles
x509 LTC_S0.pem          :     47284 cycles
x509 LTC_SS0.pem         :     36687 cycles
x509 secp384r1.pem       :   1985416 cycles
x509 secp521r1.pem       :   3287773 cycles
x509 LTC_SSS0.pem        :     25086 cycles
x509 secp224r1.pem       :    775807 cycles
```

After this patch:

[0]
```
==1043548== HEAP SUMMARY:
==1043548==     in use at exit: 0 bytes in 0 blocks
==1043548==   total heap usage: 337,244 allocs, 337,244 frees, 65,047,463 bytes allocated
```

[1]
```
x509 cert-rsa-pss.pem    :     32568 cycles
x509 LTC_CA.pem          :      5478 cycles
x509 LTC_S0.pem          :     36093 cycles
x509 LTC_SS0.pem         :     23351 cycles
x509 secp384r1.pem       :   1984030 cycles
x509 secp521r1.pem       :   3303396 cycles
x509 LTC_SSS0.pem        :     13220 cycles
x509 secp224r1.pem       :    781534 cycles
```

[0] find tests/x509 -name '*.pem' -exec valgrind --leak-check=full --show-leak-kinds=all './x509_verify' {} \+
[1] ./timing x509

Signed-off-by: Steffen Jaeckel <[email protected]>
Signed-off-by: Steffen Jaeckel <[email protected]>
... in order of likelihood of usage and/or strength.

Signed-off-by: Steffen Jaeckel <[email protected]>
Only go once through `X_descriptor[]` when calling `register_X()`

Signed-off-by: Steffen Jaeckel <[email protected]>
Before this patch it silently didn't work, now it errors out.

Signed-off-by: Steffen Jaeckel <[email protected]>
@sjaeckel sjaeckel force-pushed the some-improvements branch 3 times, most recently from e794721 to 151a655 Compare October 16, 2025 12:43
@karel-m
Copy link
Member

karel-m commented Oct 16, 2025

I am not sure about having: type = LTC_ECCSIG_RFC5656 (as I understand the "type" is rather a format specification not a flag defining whether to use deterministic signing) e.g. one might want LTC_ECCSIG_RFC5656 + LTC_ECCSIG_ETH27 at the same time.

@karel-m
Copy link
Member

karel-m commented Oct 16, 2025

ad struct ltc_ecc_sig_opts could we simplify int *recid; to int recid; ?

@sjaeckel
Copy link
Member Author

ad struct ltc_ecc_sig_opts could we simplify int *recid; to int recid; ?

Yeah, we could, but that would mean that it must be initialized to -1 if one is not interested in it (which is nearly always the case). With the current implementation via a pointer, it is disabled by default, so it'd be easier to use for the majority of users. But if you think it'd be better as int recid feel free to say so! I don't really have a hard opinion on that.

I am not sure about having: type = LTC_ECCSIG_RFC5656 (as I understand the "type" is rather a format specification not a flag defining whether to use deterministic signing) e.g. one might want LTC_ECCSIG_RFC5656 + LTC_ECCSIG_ETH27 at the same time.

I guess you're confusing RFC5656 and RFC6979 :)

@karel-m
Copy link
Member

karel-m commented Oct 17, 2025

I guess you're confusing RFC5656 and RFC6979 :)

Oh, yes. Sorry for the noise.

@karel-m
Copy link
Member

karel-m commented Oct 17, 2025

FYI current branch some-improvements seems to fail on arm and cywin (will investigate later)

sjaeckel and others added 7 commits October 17, 2025 13:29
* The oldest supported Ubuntu Version is 22.04 which comes with CMake 3.22.
* Refactor demos/CMakeLists.txt to set the list of binaries only once.

Signed-off-by: Steffen Jaeckel <[email protected]>
This reverts commit 93f5348.

You should use the CMake flag `-DCMAKE_CXX_COMPILER_LAUNCHER=ccache`
instead.

Sibling-to: libtom/libtommath#577
Signed-off-by: Steffen Jaeckel <[email protected]>
This closes #354

Signed-off-by: Steffen Jaeckel <[email protected]>
Change the default behavior of `LTC_ARGCHK()` for Release, resp.
Release+Shared Library builds to be non-fatal.

This closes #458

Signed-off-by: Steffen Jaeckel <[email protected]>
One can now define them on a per-case basis to disable them.

Signed-off-by: Steffen Jaeckel <[email protected]>
@karel-m
Copy link
Member

karel-m commented Oct 17, 2025

FYI current branch some-improvements seems to fail on arm and cywin (will investigate later)

it is a segfault (memory corruption) related to ecc_sign_hash_v2 (still not sure if in libtomcrypt or in C code of my perl bindings)

@sjaeckel
Copy link
Member Author

FYI current branch some-improvements seems to fail on arm and cywin (will investigate later)

it is a segfault (memory corruption) related to ecc_sign_hash_v2 (still not sure if in libtomcrypt or in C code of my perl bindings)

hmm

$ ./test ecc
[...]
SUCCESS: passed=1 failed=0 nop=0 duration=41.5sec real=41.5sec
$ file test                                              
test: ELF 64-bit LSB executable, ARM aarch64, version 1 (GNU/Linux), statically linked, BuildID[sha1]=8aab34cfb2c11a97baefcf2787a078587a6795ea, for GNU/Linux 4.3.0, with debug_info, not stripped

At least the tests can't reproduce it for AARCH64 with Qemu ...

@karel-m
Copy link
Member

karel-m commented Oct 17, 2025

on cywin it fails inside ecc_rfc6979_key in the first hmac_memory_multi (which is strange)

@karel-m
Copy link
Member

karel-m commented Oct 17, 2025

The fix DCIT/perl-CryptX@cccc52b

now passes tests also on cygwin + arm

It expects a pair of type `(unsigned char*,unsigned long)` and not
`(unsigned char*,unsigned int)`.

Fixes: 46fa363 ("Finish up RFC6979 ECDSA keygen")
Reported-via: #699 (comment) ff.
Signed-off-by: Steffen Jaeckel <[email protected]>
@sjaeckel
Copy link
Member Author

The fix DCIT/perl-CryptX@cccc52b

Thanks for triaging it and proposing the fix. Seeing it like that it's obvious now ...

now passes tests also on cygwin + arm

The last patch should be an equivalent solution to your proposed fix, could you please check again?

@karel-m
Copy link
Member

karel-m commented Oct 20, 2025

The last patch should be an equivalent solution to your proposed fix, could you please check again?

checked, it is good

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

Successfully merging this pull request may close these issues.

rfc6979_hash_alg question On documentation: Add list of all supported ciphers, modes, hash algorithms, curves and so on

3 participants