Skip to content

Comments

Add support for OCB, OsslCipher::block_size, and RSA helpers for implementations of RFC9580#308

Merged
simo5 merged 8 commits intolatchset:mainfrom
teythoon:justus/workwork
Jul 30, 2025
Merged

Add support for OCB, OsslCipher::block_size, and RSA helpers for implementations of RFC9580#308
simo5 merged 8 commits intolatchset:mainfrom
teythoon:justus/workwork

Conversation

@teythoon
Copy link
Contributor

Description

This patch series adds functionality required to port Sequoia PGP to ossl. First, we add support for OCB, which is the MTI AEAD mode of RFC9580. Second, we tweak ossl's notion of block size so that AEAD constructions are treated the same way as stream ciphers by OsslCipher::buffer_size, and we expose the block size as OsslCipher::block_size. Third, we add some helper functions to load and store RSA keys using the OpenPGP wire format.

Checklist

  • Test suite updated with functionality tests
  • Test suite updated with negative tests
  • Rustdoc string were added or updated
  • CHANGELOG and/or other documentation added or updated
  • This is not a code change

Reviewer's checklist:

  • Any issues marked for closing are fully addressed
  • There is a test suite reasonably covering new functionality or modifications
  • This feature/change has adequate documentation added
  • A changelog entry is added if the change is significant
  • Code conform to coding style that today cannot yet be enforced via the check style test
  • Commits have short titles and sensible text
  • Doc string are properly updated

@simo5
Copy link
Member

simo5 commented Jul 29, 2025

I really do not like to expose BIGNUM, and even less I want to expose functions that are not side-channel safe like is done in these patches.

Given RFC5980 is specifically an OpenPGP RFC, I think you should keep that code in sequoia.

@teythoon
Copy link
Contributor Author

The patches do not expose BIGNUM. I added the helpers in order to not have to expose BIGNUM.

Keeping the code in Sequoia would require doing the bignum arithmetic in Sequoia, which requires exposing (some parts of) the BIGNUM interface.

I distinctively recall us discussing this issue, and us agreeing that this is the way to go. If I misunderstood anything, e.g. if there is a way to keep this code in Sequoia while not exposing parts of the BIGNUM interface, please advise.

@Jakuje
Copy link
Contributor

Jakuje commented Jul 29, 2025

Ok, so the related SSH issue we had was the following, which is supposivelly fixed now (but we still have a code calculating this in libssh and OpenSSH to work with older OpenSSLs) is here: openssl/openssl#21826

I am not sure if they added added calculation of the missing parameters I asked, or any. I also did not read deeply through this code to verify its the same parameters that we need here.

Skimming through the openssh code, they seem to be adding at least a flags to make the private exponent operations constant time. Would this help to introduce here?

https://github.com/openssh/openssh-portable/blob/17fa6cd10a26e193bb6f65d21264d2fe553bcd87/ssh-rsa.c#L336

@Jakuje
Copy link
Contributor

Jakuje commented Jul 29, 2025

FYI the rpm build is fixed if you rebase your changes on top of current master.

Copy link
Member

@simo5 simo5 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok i realize this is the lesser evil than exposing the whole of bignum out, even though I do not particularly like the fact we need to do this at all.

What I really do not like is the checked_* functions as they encourage more direct use of bignum even if internally. I would like to see those functions folded back into from_dpq() and confined there.

If "u" is always computed, it may make sense to also always fold that computation into from_dpq() and just save the value for later retrieval instead of computing it on the fly, otherwise it needs a better name.

I think I would like to see from_dpq() behind a feature called something like "rfc9580"

I think we also need some doc strings that should warn that the computations performed by from_dpq() are not side-channel free, therefore this function should not be used in software that can call it in response to network operations such that an attacker would be able to measure the time it takes to perform them.

@simo5
Copy link
Member

simo5 commented Jul 29, 2025

Oh and of course I think we need at least one test vector that ensures the from_dpq() function is actually returning the correct values.

Copy link
Member

@simo5 simo5 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@simo5 simo5 merged commit 461010b into latchset:main Jul 30, 2025
49 checks passed
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.

3 participants