gh-libtom/libtomcrypt
Watch
1
0
Fork
You've already forked libtomcrypt
 0
Code Issues 42 Pull requests 17 Releases 9 Wiki Activity

Some improvements #699

Merged
sjaeckel merged 9 commits from some-improvements into develop 2025-11-18 17:22:08 -06:00
Conversation 27 Commits 9 Files changed 19 +406 -241
sjaeckel commented 2025-10-03 04:22:31 -05:00 (Migrated from github.com)

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

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
levitte (Migrated from github.com) reviewed 2025-10-03 04:22:31 -05:00
karel-m (Migrated from github.com) reviewed 2025-10-03 04:22:31 -05:00
karel-m commented 2025-10-03 05:14:02 -05:00 (Migrated from github.com)

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).

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 commented 2025-10-03 08:45:30 -05:00 (Migrated from github.com)

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

> 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 :relaxed: [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 commented 2025-10-05 11:52:37 -05:00 (Migrated from github.com)

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

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
karel-m commented 2025-10-16 08:20:36 -05:00 (Migrated from github.com)

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 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 commented 2025-10-16 08:25:24 -05:00 (Migrated from github.com)

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

ad `struct ltc_ecc_sig_opts` could we simplify `int *recid;` to `int recid;` ?
sjaeckel commented 2025-10-17 04:13:21 -05:00 (Migrated from github.com)

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 :)

> 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 commented 2025-10-17 05:08:14 -05:00 (Migrated from github.com)

I guess you're confusing RFC5656 and RFC6979 :)

Oh, yes. Sorry for the noise.

> I guess you're confusing RFC5656 and RFC6979 :) Oh, yes. Sorry for the noise.
karel-m commented 2025-10-17 05:27:02 -05:00 (Migrated from github.com)

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

FYI current branch `some-improvements` seems to fail on arm and cywin (will investigate later)
karel-m commented 2025-10-17 07:34:12 -05:00 (Migrated from github.com)

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)

> 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 commented 2025-10-17 11:14:22 -05:00 (Migrated from github.com)

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 ...

> > 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 commented 2025-10-17 11:51:40 -05:00 (Migrated from github.com)

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

on cywin it fails inside `ecc_rfc6979_key` in the first `hmac_memory_multi` (which is strange)
karel-m commented 2025-10-17 12:08:31 -05:00 (Migrated from github.com)

The fix github.com/DCIT/perl-CryptX@cccc52bb62

now passes tests also on cygwin + arm

The fix https://github.com/DCIT/perl-CryptX/commit/cccc52bb62e90900afd0317db7e4ffb19339a859 now passes tests also on cygwin + arm
sjaeckel commented 2025-10-20 10:49:00 -05:00 (Migrated from github.com)

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?

> The fix [DCIT/perl-CryptX@cccc52b](https://github.com/DCIT/perl-CryptX/commit/cccc52bb62e90900afd0317db7e4ffb19339a859) 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 commented 2025-10-20 12:41:40 -05:00 (Migrated from github.com)

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

checked, it is good

> The last patch should be an equivalent solution to your proposed fix, could you please check again? checked, it is good
sjaeckel (Migrated from github.com) reviewed 2025-10-23 05:22:58 -05:00
sjaeckel (Migrated from github.com) left a comment

If you're fine with those latest changes [0] I'll add the same macro trickery for the deprecated RSA APIs as well, properly deprecate all the remaining APIS, and then we'll only have to update the docs...

[0] https://github.com/libtom/libtomcrypt/pull/699/files/baf71ad0544d1b1f03b5b644203b6afbe43b9feb..b78da6e65607c23f852ce78583e90025cd2817d9

If you're fine with those latest changes [0] I'll add the same macro trickery for the deprecated RSA APIs as well, properly deprecate all the remaining APIS, and then we'll only have to update the docs... [0] https://github.com/libtom/libtomcrypt/pull/699/files/baf71ad0544d1b1f03b5b644203b6afbe43b9feb..b78da6e65607c23f852ce78583e90025cd2817d9
src/headers/tomcrypt_pk.h
sjaeckel (Migrated from github.com) commented 2025-10-23 03:50:08 -05:00

This would allow us to extend functionality without breaking API&ABI in the future.

Is this really necessary for RSA?

Should some ECC struct get something like that?

This would allow us to extend functionality without breaking API&ABI in the future. Is this really necessary for RSA? Should some ECC struct get something like that?
karel-m commented 2025-11-02 03:34:44 -06:00 (Migrated from github.com)

The old RSA style let you pass two values separately: int mgf_hash and int lparam_hash. But the new style has only one value: int hash_idx (see rsa_args_to_op_params).

The old RSA style let you pass two values separately: `int mgf_hash` and `int lparam_hash`. But the new style has only one value: int `hash_idx` (see `rsa_args_to_op_params`).
sjaeckel commented 2025-11-02 08:10:15 -06:00 (Migrated from github.com)

The old RSA style let you pass two values separately: int mgf_hash and int lparam_hash. But the new style has only one value: int hash_idx (see rsa_args_to_op_params).

True. That second parameter was only introduced in #612 and in order to make the API backwards compatible again I've limited the new functionality to the v2 API.

I thought if we already introduce new APIs, we can also do the full deprecation dance. We're not going to be ABI compatible, but the API's would stay the same.

> The old RSA style let you pass two values separately: `int mgf_hash` and `int lparam_hash`. But the new style has only one value: int `hash_idx` (see `rsa_args_to_op_params`). True. That second parameter was only introduced in #612 and in order to make the API backwards compatible again I've limited the new functionality to the v2 API. I thought if we already introduce new APIs, we can also do the full deprecation dance. We're not going to be ABI compatible, but the API's would stay the same.
karel-m commented 2025-11-02 08:23:38 -06:00 (Migrated from github.com)

After few hours I am still unable to switch my perl bindings to the new RSA API. I make it to compile but it fails during some operations (enc/dec, sign/verify) and it is not easy for me to debug why.

I have chosen a way like this:

          ltc_rsa_op_parameters rsa_oparams = {
            .prng = &self->pstate,
            .wprng = self->pindex,
            .padding = LTC_PKCS_1_OAEP,
            .u.crypt.lparam = lparam_ptr,
            .u.crypt.lparamlen = (unsigned long)lparam_len,
            .params.hash_alg = hash_descriptor[lparam_hash_id].name,
            .params.mgf1_hash_alg = hash_descriptor[mgf_hash_id].name,
            .params.saltlen = 0,
            .params.pss_oaep = 1,
          };
          
          rv = rsa_encrypt_key_v2(data_ptr, (unsigned long)data_len, buffer, &buffer_len, &rsa_oparams, &self->key);

but still getting errors like "Invalid PK key or key type specified for function" + on other places errors like "Invalid hash specified" etc.

Maybe I still did not fully understand the new concept (it has definitely much steeper learning curve than the old one).

After few hours I am still unable to switch my perl bindings to the new RSA API. I make it to compile but it fails during some operations (enc/dec, sign/verify) and it is not easy for me to debug why. I have chosen a way like this: ``` ltc_rsa_op_parameters rsa_oparams = { .prng = &self->pstate, .wprng = self->pindex, .padding = LTC_PKCS_1_OAEP, .u.crypt.lparam = lparam_ptr, .u.crypt.lparamlen = (unsigned long)lparam_len, .params.hash_alg = hash_descriptor[lparam_hash_id].name, .params.mgf1_hash_alg = hash_descriptor[mgf_hash_id].name, .params.saltlen = 0, .params.pss_oaep = 1, }; rv = rsa_encrypt_key_v2(data_ptr, (unsigned long)data_len, buffer, &buffer_len, &rsa_oparams, &self->key); ``` but still getting errors like "Invalid PK key or key type specified for function" + on other places errors like "Invalid hash specified" etc. Maybe I still did not fully understand the new concept (it has definitely much steeper learning curve than the old one).
sjaeckel commented 2025-11-02 08:35:04 -06:00 (Migrated from github.com)

After few hours I am still unable to switch my perl bindings to the new RSA API. I make it to compile but it fails during some operations (enc/dec, sign/verify) and it is not easy for me to debug why.

ugh, sorry! The tests seem to work fine for me.

I've just pushed my WIP state with the updated macros etc.

> After few hours I am still unable to switch my perl bindings to the new RSA API. I make it to compile but it fails during some operations (enc/dec, sign/verify) and it is not easy for me to debug why. ugh, sorry! The tests seem to work fine for me. I've just pushed my WIP state with the updated macros etc.
sjaeckel commented 2025-11-02 08:36:55 -06:00 (Migrated from github.com)

but still getting errors like "Invalid PK key or key type specified for function" + on other places errors like "Invalid hash specified" etc.

It could absolutely be that the generic validity checks don't handle everything correctly, I've only tested against the built-in set of tests... 😬

> but still getting errors like "Invalid PK key or key type specified for function" + on other places errors like "Invalid hash specified" etc. It could absolutely be that the generic validity checks don't handle everything correctly, I've only tested against the built-in set of tests... :grimacing:
karel-m commented 2025-11-02 09:37:10 -06:00 (Migrated from github.com)

shouldn't the s_rsa_key_valid_pss_algs be rather s_rsa_key_valid_pss_oaep_algs and allow also padding LTC_PKCS_1_OAEP?

shouldn't the `s_rsa_key_valid_pss_algs` be rather `s_rsa_key_valid_pss_oaep_algs` and allow also padding `LTC_PKCS_1_OAEP`?
karel-m commented 2025-11-02 09:47:52 -06:00 (Migrated from github.com)

The simple example I am not able to implement in the new RSA API (sorry for just a code fragment extracted from my perl bindings):

/* load the key in DER format */
rsa_import(data, (unsigned long)data_len, &self->key);

/* try to encrypt */
ltc_rsa_op_parameters rsa_oparams = {
  .prng = &self->pstate,
  .wprng = self->pindex,
  .padding = LTC_PKCS_1_OAEP,
  .u.crypt.lparam = lparam_ptr,
  .u.crypt.lparamlen = (unsigned long)lparam_len,
  .params.hash_alg = hash_descriptor[lparam_hash_id].name,
  .params.mgf1_hash_alg = hash_descriptor[mgf_hash_id].name,
  .params.saltlen = 0,
  .params.pss_oaep = 1,
};
rv = rsa_encrypt_key_v2(data_ptr, (unsigned long)data_len, buffer, &buffer_len, &rsa_oparams, &self->key);
The simple example I am not able to implement in the new RSA API (sorry for just a code fragment extracted from my perl bindings): ``` /* load the key in DER format */ rsa_import(data, (unsigned long)data_len, &self->key); /* try to encrypt */ ltc_rsa_op_parameters rsa_oparams = { .prng = &self->pstate, .wprng = self->pindex, .padding = LTC_PKCS_1_OAEP, .u.crypt.lparam = lparam_ptr, .u.crypt.lparamlen = (unsigned long)lparam_len, .params.hash_alg = hash_descriptor[lparam_hash_id].name, .params.mgf1_hash_alg = hash_descriptor[mgf_hash_id].name, .params.saltlen = 0, .params.pss_oaep = 1, }; rv = rsa_encrypt_key_v2(data_ptr, (unsigned long)data_len, buffer, &buffer_len, &rsa_oparams, &self->key); ```
sjaeckel commented 2025-11-02 10:05:54 -06:00 (Migrated from github.com)

I'll have a look into it later, but please remove .params.pss_oaep = 1, and it works ... 🙈

I'll have a look into it later, but please remove ` .params.pss_oaep = 1,` and it works ... :see_no_evil:
karel-m commented 2025-11-02 13:06:42 -06:00 (Migrated from github.com)

I'll have a look into it later, but please remove .params.pss_oaep = 1, and it works ...

It worked. But IMO a bit surprising to me (as the whole concept of key->params / check->params / ltc_rsa_op_parameters ...)

Anyway, still bunch of other failures (will investigate later).

> I'll have a look into it later, but please remove .params.pss_oaep = 1, and it works ... It worked. But IMO a bit surprising to me (as the whole concept of key->params / check->params / ltc_rsa_op_parameters ...) Anyway, still bunch of other failures (will investigate later).
sjaeckel commented 2025-11-03 07:20:12 -06:00 (Migrated from github.com)

[...] a bit surprising to me (as the whole concept of key->params / check->params / ltc_rsa_op_parameters ...)

Yeah, a bit, but not really. The key->params exist because there's the possibility to restrict the PKCS#1 version via X.509 certificates. The re-use of ltc_rsa_parameters inside ltc_rsa_op_parameters, came out of laziness because it's kind of shared details (minus the pss_oaep field with isn't required there).

Regular RSA keys can be used with all PKCS#1 versions.

Restricted RSA keys ("PSS-keys") must only be used with PKCS#1 versions > 1.5, and come with a set of parameters. There's already rsa_import_x509.c::rsa_decode_parameters(), but that one isn't used yet, resp. I'll have to check whether it should be. I've other stuff to finish before continuing on this.

> [...] a bit surprising to me (as the whole concept of key->params / check->params / ltc_rsa_op_parameters ...) Yeah, a bit, but not really. The `key->params` exist because there's the possibility to restrict the PKCS#1 version via X.509 certificates. The re-use of `ltc_rsa_parameters` inside `ltc_rsa_op_parameters`, came out of laziness because it's kind of shared details (minus the `pss_oaep` field with isn't required there). Regular RSA keys can be used with all PKCS#1 versions. Restricted RSA keys ("PSS-keys") must only be used with PKCS#1 versions > 1.5, and come with a set of parameters. There's already `rsa_import_x509.c::rsa_decode_parameters()`, but that one isn't used yet, resp. I'll have to check whether it should be. I've other stuff to finish before continuing on this.
Sign in to join this conversation.
Reviewers
No reviewers
levitte
karel-m
Labels
Clear labels   
bug

   
duplicate

   
enhancement

   
feature

   
help wanted

   
improve API

   
invalid

   
needs documentation

   
not-planned
This ticket is not planned to be implemented and/or fixed from the core team.

  
rtfm

   
stale

   
support

   
wishlist
No labels
bug
duplicate
enhancement
feature
help wanted
improve API
invalid
needs documentation
not-planned
rtfm
stale
support
wishlist
Milestone
Clear milestone
No items
No milestone
next
Assignees
Clear assignees
No assignees
1 participant
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set

Reference
gh-libtom/libtomcrypt!699
No description provided.