openssh bugs - Apr 2023 - [Bug 3561] New: Open SSH does not support 1-byte structure packing on non-windows systems for PKCS11

https://bugzilla.mindrot.org/show_bug.cgi?id=3561

            Bug ID: 3561
           Summary: Open SSH does not support 1-byte structure packing on
                    non-windows systems for PKCS11
           Product: Portable OpenSSH
           Version: 9.3p1
          Hardware: Other
                OS: All
            Status: NEW
          Severity: enhancement
          Priority: P5
         Component: Miscellaneous
          Assignee: unassigned-bugs at mindrot.org
          Reporter: doug.springer at idahoscientific.com

The PKCS11 specification is very clear on byte packing (1 byte), but
openssh does not even attempt to do so for anything besides Windows.
Instead, it relies on the compiler to set the size of CK_ULONG, CK_LONG
to unsigned long, which may be 32 or 64 depending on system and
compiler. While the PKCS11 API is painful in it's original form, it is
a very simple matter to add a definition for the appropriate packed
syntax, and define it for the compiler, or rely on a pragma.

This should at least be configurable at build time. Although many
implementations of PKCS11 on non-windows systems also do this, is that
really a good reason to ignore the standard? There really isn't a
standard at that point. This is especially troubling since the API
allows many places to fail.

-- 
You are receiving this mail because:
You are watching the assignee of the bug.

https://bugzilla.mindrot.org/show_bug.cgi?id=3561

--- Comment #1 from RickyDoug <doug.springer at idahoscientific.com> ---
This is actually a confused report, mixing two different issues. One is
the lack of byte packing. The second is depending on the compiler to
set the size of CK_ULONG instead of using stdint.h to specifically set
the type.

-- 
You are receiving this mail because:
You are watching the assignee of the bug.

https://bugzilla.mindrot.org/show_bug.cgi?id=3561

Damien Miller <djm at mindrot.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |djm at mindrot.org

--- Comment #2 from Damien Miller <djm at mindrot.org> ---
I don't understand. The definition of CK_ULONG etc is exactly what is
specified in the PKCS11 standard:
https://docs.oasis-open.org/pkcs11/pkcs11-base/v2.40/os/pkcs11-base-v2.40-os.html#_Toc319287635

It might be clearer if you provided a link to the section of the
standard we are getting wrong.

-- 
You are receiving this mail because:
You are watching the assignee of the bug.
You are watching someone on the CC list of the bug.

https://bugzilla.mindrot.org/show_bug.cgi?id=3561

--- Comment #3 from RickyDoug <doug.springer at idahoscientific.com> ---
My apologies. Please ignore the reference to CK_ULONG...that is another
issue. Perhaps a re-state is in order:

The PKCS11 specification is very clear on byte packing (1 byte), but
openssh does not even attempt to do so for anything besides Windows. 

http://docs.oasis-open.org/pkcs11/pkcs11-base/v2.40/os/pkcs11-base-v2.40-os.html#_Toc416959683

 While the PKCS11 API is painful in it's original form, it is a very
simple matter to add a definition for the appropriate packed syntax,
and define it for the compiler, or rely on a pragma, such as the gcc
pragma pack.

This should at least be configurable at build time. Although many
implementations of PKCS11 on non-windows systems also do this, is that
really a good reason to ignore the standard? There really isn't a
standard at that point. This is especially troubling since the API
allows many places to fail.

-- 
You are receiving this mail because:
You are watching the assignee of the bug.
You are watching someone on the CC list of the bug.

https://bugzilla.mindrot.org/show_bug.cgi?id=3561

--- Comment #4 from Damien Miller <djm at mindrot.org> ---
The spec might say use 1-byte packing, but I can't find a Unix/Linux
implementation that actually does this. 

OpenSC doesn't:
https://github.com/OpenSC/libp11/blob/master/src/pkcs11.h
WolfSSL doesn't:
https://github.com/wolfSSL/wolfssl/blob/master/wolfssl/wolfcrypt/pkcs11.h
Android doesn't:
https://android.googlesource.com/platform/hardware/ti/omap4-aah/+/master/security/tf_sdk/include/pkcs11.h

and the example header file from OASIS themselves is at best ambiguous:

http://docs.oasis-open.org/pkcs11/pkcs11-base/v2.40/errata01/csprd01/include/pkcs11-v2.40/pkcs11.h

No incompatibilities in OpenSSH have been reported so far relating to
this, and you haven't described what problems you're experiencing and
what platform/OS you're using.
> Although many implementations of PKCS11 on non-windows systems also
> do this, is that really a good reason to ignore the standard?
Yes, is it actually a great reason to ignore the standard. Following it
would make us incompatible with the overwhelming majority of software
that our users expect us to interoperate with.

-- 
You are receiving this mail because:
You are watching the assignee of the bug.
You are watching someone on the CC list of the bug.

https://bugzilla.mindrot.org/show_bug.cgi?id=3561

--- Comment #5 from RickyDoug <doug.springer at idahoscientific.com> ---
I totally agree with you that there are few, if any that even
build-time support it and none that release binaries packed, at least
that I have found. 

The real issue is someone copied a header file without packing that has
now made it 'standard' in the Linux realm. Unfortunately every project
copied the same file or something very similar, and now we have
non-compliant implementations all over the place. The PKCS11 standard
is very clear that packing is required. An example header does not
constitute a standard. Just because whoever wrote the API didn't know
anything about Unix and basically said so in the header does not make
the requirement ambiguous.

BTW, the packing control wouldn't be in the pkcs11.h file, it's usually
in the header that includes it. The google test suite is the only
project that thought about this and provided a way.

Here's a simple, two line solution:
https://github.com/google/pkcs11test/blob/dev/pkcs11-env.h

And really that is all this bug report is about... the *ability* to
build packed structures so the *possibility* exits somewhere in the
future for the project to be compliant. For other users using source
code, the project can be compliant now.

-- 
You are receiving this mail because:
You are watching someone on the CC list of the bug.
You are watching the assignee of the bug.

https://bugzilla.mindrot.org/show_bug.cgi?id=3561

Damien Miller <djm at mindrot.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|NEW                         |RESOLVED
         Resolution|---                         |WONTFIX

--- Comment #6 from Damien Miller <djm at mindrot.org> ---
It's not just one header that was copied though, there are (at least)
four different implementations that have arrived at the same situation
independently. The defacto standard is clear here.

Adding an option to OpenSSH to control pkcs11.h structure packing is
definitely not without cost and it's not clear whether we can make it
work for all the compiler toolchains that we support.

Unless and until there is some PKCS11 module that actually uses packed
structs on Unix, this option would just be a way to break ones
installation in subtle and scary ways.

Given this, we won't be implementing structure packing in OpenSSH. If
an ecosystem of PKCS11 modules that actually require packing emerges,
then feel free to reopen this bug and we can reconsider.

-- 
You are receiving this mail because:
You are watching the assignee of the bug.
You are watching someone on the CC list of the bug.