Page MenuHomeFreeBSD
Differential D40296

dumpon(8): support OpenSSL 3
Needs ReviewPublic
Actions

Authored by ngie on May 27 2023, 2:56 AM.
Tags
None
Referenced Files
Unknown Object (File)
Thu, Oct 1, 10:11 AM
Unknown Object (File)
Fri, Sep 25, 3:56 PM
Unknown Object (File)
Mon, Sep 21, 12:18 PM
Unknown Object (File)
Sun, Sep 20, 10:14 AM
Unknown Object (File)
Mon, Sep 14, 8:16 AM
Unknown Object (File)
Sun, Sep 13, 1:03 AM
Unknown Object (File)
Fri, Sep 11, 7:30 PM
Unknown Object (File)
Thu, Sep 10, 8:55 AM
View All Files
Subscribers
delphij
imp

Details

Reviewers
emaste
khorben
Summary

This change moves away from the 3.0 deprecated APIs such that the code
no longer needs to explicitly request OpenSSL 1.1 API support to compile
cleanly with -Werror.

While here, axe pre-OpenSSL 1.1 ifdefs: FreeBSD has not "shipped"
OpenSSL 1.0 for some time, so there's no need to keep the ifdef soup
around.

MFC after: 1 month

Test Plan

Tested with both OpenSSL 1.1 from base and 3.0 (from ports). Both instances fail when trying to set the kernelsdump ioctl because the device I was using is md(4) based:

# rsa.public generated with the following 2 commands:
# - openssl genrsa -out rsa.private 2048
# - openssl rsa -in rsa.priv -out rsa.public -pubout -outform PEM
$ cat rsa.public 
-----BEGIN PUBLIC KEY-----
MIIBIjANBgkqhkiG9w0BAQEFAAOCAQ8AMIIBCgKCAQEAz/Kc/3eSS4uCoEBhzsI7
P3Vy7+Ar/JcKHUL6nlrKwOECOTaQf63KhIxvHh3wsBIpiqIDoN0MgumECCdhco8G
mdwcFzf5zavq7LsYPsfY79nEUgrtqNPxS0hekNhg49RVjRBFdm++CL4c1HHoj/QO
V2+SDDPO+2/0ZlfZdtbglYNTB+jh3YpgXlh0ap5axpY/s47fcJVhHGDFBChwaGPd
0Kb16XCqmHyCAx5q2W+haBDwEXhXSBSbFMCB5luItteoG/66gtUK1fpps9Jmx0e8
MHxnkFYctVAbDw/p1boyQQWQ7o9FyLyKWf8/hx6RPyA9kV3oFPYhE0Pf/YZOzRcU
WwIDAQAB
-----END PUBLIC KEY-----
$ sudo dumpon -vvv -k ./rsa.public /dev/md0 
dumpon: ioctl(DIOCSKERNELDUMP): Operation not supported
$ sudo `make -V.OBJDIR`/dumpon -vvv -k ./rsa.public /dev/md0 
dumpon: ioctl(DIOCSKERNELDUMP): Operation not supported

The above public key was

Diff Detail

Repository
rG FreeBSD src repository
Lint
Lint Skipped
Unit
Tests Skipped
Build Status
Buildable 51729
Build 48620: arc lint + arc unit

Event Timeline

ngie created this revision.May 27 2023, 2:56 AM
Herald added a subscriber: imp. · View Herald TranscriptMay 27 2023, 2:56 AM
ngie requested review of this revision.May 27 2023, 2:56 AM
Harbormaster completed remote builds in B51729: Diff 122505.May 27 2023, 2:56 AM
ngie added a reviewer: khorben.May 27 2023, 2:57 AM
ngie added a subscriber: delphij.
ngie edited the test plan for this revision. (Show Details)May 27 2023, 3:02 AM
ngie edited the test plan for this revision. (Show Details)
delphij added a comment.May 27 2023, 6:21 PM

I think these OPENSSL_VERSION_NUMBER >= 0x30000000L code will actually work with OpenSSL 1.1?

Could you please try if the code will still work after stripping the old code with unifdef -DOPENSSL_VERSION_NUMBER=0x30000000L, and remove the old code if that's the case? Assuming it worked, it would also allow us to remove some intermediate variables and make the diff smaller.

ngie updated this revision to Diff 122521.May 27 2023, 8:57 PM
ngie edited the test plan for this revision. (Show Details)

Remove all pre-3.0 related ifdefs.

All but EVP_PKEY_get_size (which is just an alias to EVP_PKEY_size in 3.0)
can be found/used in 1.1.

Eliminate all possible intermediate variables.

Harbormaster completed remote builds in B51737: Diff 122521.May 27 2023, 8:57 PM
delphij added inline comments.May 27 2023, 11:10 PM
sbin/dumpon/dumpon.c
316

I think EVP_PKEY_bits(pubkey) would be more appropriate here (it's the actual bits of the RSA key). The security bits is the estimate of security strength equivalent and is not linear to key size.

ngie updated this revision to Diff 122525.May 28 2023, 12:37 AM

Update values and messages used when testing pubkey limits

Make error messages associated with the required minimum and maximum
number of RSA pubkeys more direct.

Use functions and computed values that match the desired end-state without
having to convert too much between bits/size.

Harbormaster completed remote builds in B51739: Diff 122525.May 28 2023, 12:37 AM
ngie updated this revision to Diff 122526.May 28 2023, 12:41 AM

Make the minimum required RSA bits 256, not 112.

The previous code stated that the minimum number of bits must be 2048 for
pre-1.1 builds. Honor the 256 bits requirement, as the 112 minimum (which
results in 896 bytes) -- a number which seems strange to use as a lower
bound for RSA algorithmic complexity.

Harbormaster completed remote builds in B51740: Diff 122526.May 28 2023, 12:42 AM
ngie updated this revision to Diff 122527.May 28 2023, 12:47 AM

Consistently use bytes or bits in calculations

Use bytes when talking about RSA pubkey length.

Harbormaster completed remote builds in B51741: Diff 122527.May 28 2023, 12:47 AM
ngie updated this revision to Diff 122528.May 28 2023, 12:54 AM

Include appropriate headers for new APIs in use

Harbormaster completed remote builds in B51742: Diff 122528.May 28 2023, 12:54 AM
ngie added a comment.May 28 2023, 2:08 AM

I've been going back and forth a lot over the wording and APIs in use because I've been refining my understanding upon reading the manpages and tried to get things consistent across the board to aid with program UX 😅.
The existing code was also misleading/incorrect in terms of how the limits were calculated and enforced, too: it should have been 256, not 112 -- I have no idea why someone chose the number 112.

delphij added inline comments.May 28 2023, 3:10 AM
sbin/dumpon/dumpon.c
236

This has to be in bits, not in bytes (regardless if we are requiring a security strength (the current code is doing) or a minimum size of key); using bytes is very confusing for readers as key sizes are always measured in bits.

The current code is using a security strength bits, as demanded in NIST SP800-57 (see reasoning in the comment below). Conceptually it's saying "This key is as strong as X bits symmetric cipher" regardless of the actual key size.

The proposed change would assume that the underlying key is a RSA key and requiring a minimal key size of 2048 bits (represented as 256 bytes), according to table 2 (page 54), the corresponding security strength is 112 bits, so it's equivalent to the current code.

I don't really have a strong opinion on which approach would be better here, but personally I prefer using the current one (use security bits instead of key length) because a) it's closer to the standard text (112 bits of strength acceptable up to 2030), and b) we don't have to change the code to support other public key encryption methods, because they would be mapped to the same strength bits table.

321–322

(see comment above; compare with bits not bytes. Personally I'd prefer using the security bits as we do today).

326

This should be EVP_PKEY_bits(pubkey), or EVP_PKEY_size(pubkey) * 8.

ngie added inline comments.May 28 2023, 5:56 AM
sbin/dumpon/dumpon.c
236

This is incredibly helpful/enlightening context; I'll digest this and mold the current code into what it needs to be.

I'll be sure to add references to the other NIST documents for further reading.

emaste mentioned this in D40353: dumpon: update OpenSSL initialization call.May 31 2023, 4:33 PM
emaste added inline comments.Jun 2 2023, 1:41 AM
sbin/dumpon/dumpon.c
612

ERR_load_crypto_strings is deprecated as of OpenSSL 1.1; see D40353

ngie added inline comments.Jun 2 2023, 2:22 AM
sbin/dumpon/dumpon.c
612

Good catch!

Revision Contents
Changeset List

PathSize
sbin/
dumpon/
83 lines

Diff 122505

View Options

sbin/dumpon/dumpon.c

Loading...