Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support for EC keys, fix Attributes processing #369

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

ya-mouse
Copy link

This change adds proper handling for EC keys (addressing #303) as well as proper RSA parameters calculation.

NOTE: For instance, openssl expects that EC params (curve OID) and points are in specific uncompressed and padded format. In Go encoding EC_POINTS be like:

case C.CKA_EC_POINT:
    encodedPoint, err := encodeECPoint(key.Curve, key.X, key.Y)
    ...
    setValue(attr, unsafe.Pointer(&encodedPoint[0]), C.CK_ULONG(len(encodedPoint)))

func encodeECPoint(curve elliptic.Curve, x, y *big.Int) ([]byte, error) {
        fieldSize := (curve.Params().BitSize + 7) / 8
        xPadded := make([]byte, fieldSize)
        yPadded := make([]byte, fieldSize)
        copy(xPadded[fieldSize-len(x.Bytes()):], x.Bytes())
        copy(yPadded[fieldSize-len(y.Bytes()):], y.Bytes())

        ecPoint := append([]byte{0x04}, append(xPadded, yPadded...)...)

        return asn1.Marshal(asn1.RawValue{
                Tag:   asn1.TagOctetString,
                Bytes: ecPoint,
        })
}

I faced this issue when implementing fake custom backend using regular PEM/DER files or passing signature produced by AWS KMS.
Perhaps, as later improvement we can try to instantiate specific signature from DER bytes and then repack params into expected format. Now this is offloaded to the backend.

The logging function moved away from C_Initialize under ctor when library is being loaded. This allows to start logging custom backend initialization. Also the log init function reworked to remove log lines duplication by using tracing::subscriber::set_global_default(...).

Also there is a pretty strange bug when using OpenSSL when it calls C_CloseAllSessions and C_Finalize.

When using #[instrument] trait
$ openssl pkey -inform engine -engine pkcs11 -in pkcs11:model=software;manufacturer=google;serial=0000000000000000;token=custom;object=ec -pubout -text

thread '<unnamed>' panicked at /rustc/f6e511eec7342f59a25f7c0534f1dbea00d01b14/library/std/src/thread/local.rs:260:26:
cannot access a Thread Local Storage value during or after destruction: AccessError
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
thread '<unnamed>' panicked at core/src/panicking.rs:221:5:
panic in a function that cannot unwind
stack backtrace:
   0:     0x7da8a5fbffaa - std::backtrace_rs::backtrace::libunwind::trace::h99efb0985cae5d78
                               at /rustc/f6e511eec7342f59a25f7c0534f1dbea00d01b14/library/std/src/../../backtrace/src/backtrace/libunwind.rs:116:5
   1:     0x7da8a5fbffaa - std::backtrace_rs::backtrace::trace_unsynchronized::he2c1aa63b3f7fad8
                               at /rustc/f6e511eec7342f59a25f7c0534f1dbea00d01b14/library/std/src/../../backtrace/src/backtrace/mod.rs:66:5
   2:     0x7da8a5fbffaa - std::sys::backtrace::_print_fmt::h8a221d40f5e0f88b
                               at /rustc/f6e511eec7342f59a25f7c0534f1dbea00d01b14/library/std/src/sys/backtrace.rs:66:9
   3:     0x7da8a5fbffaa - <std::sys::backtrace::BacktraceLock::print::DisplayBacktrace as core::fmt::Display>::fmt::h304520fd6a30aa07
                               at /rustc/f6e511eec7342f59a25f7c0534f1dbea00d01b14/library/std/src/sys/backtrace.rs:39:26
   4:     0x7da8a5fe62eb - core::fmt::rt::Argument::fmt::h5da9c218ec984eaf
                               at /rustc/f6e511eec7342f59a25f7c0534f1dbea00d01b14/library/core/src/fmt/rt.rs:177:76
   5:     0x7da8a5fe62eb - core::fmt::write::hf5713710ce10ff22
                               at /rustc/f6e511eec7342f59a25f7c0534f1dbea00d01b14/library/core/src/fmt/mod.rs:1178:21
   6:     0x7da8a5fbd053 - std::io::Write::write_fmt::hda708db57927dacf
                               at /rustc/f6e511eec7342f59a25f7c0534f1dbea00d01b14/library/std/src/io/mod.rs:1823:15
   7:     0x7da8a5fc1292 - std::sys::backtrace::BacktraceLock::print::hbcdbec4d97c91528
                               at /rustc/f6e511eec7342f59a25f7c0534f1dbea00d01b14/library/std/src/sys/backtrace.rs:42:9
   8:     0x7da8a5fc1292 - std::panicking::default_hook::{{closure}}::he1ad87607d0c11c5
                               at /rustc/f6e511eec7342f59a25f7c0534f1dbea00d01b14/library/std/src/panicking.rs:266:22
   9:     0x7da8a5fc0efe - std::panicking::default_hook::h81c8cd2e7c59ee33
                               at /rustc/f6e511eec7342f59a25f7c0534f1dbea00d01b14/library/std/src/panicking.rs:293:9
  10:     0x7da8a5fc1b1f - std::panicking::rust_panic_with_hook::had2118629c312a4a
                               at /rustc/f6e511eec7342f59a25f7c0534f1dbea00d01b14/library/std/src/panicking.rs:797:13
  11:     0x7da8a5fc17d3 - std::panicking::begin_panic_handler::{{closure}}::h7fa5985d111bafa2
                               at /rustc/f6e511eec7342f59a25f7c0534f1dbea00d01b14/library/std/src/panicking.rs:664:13
  12:     0x7da8a5fc0489 - std::sys::backtrace::__rust_end_short_backtrace::h704d151dbefa09c5
                               at /rustc/f6e511eec7342f59a25f7c0534f1dbea00d01b14/library/std/src/sys/backtrace.rs:170:18
  13:     0x7da8a5fc1494 - rust_begin_unwind
                               at /rustc/f6e511eec7342f59a25f7c0534f1dbea00d01b14/library/std/src/panicking.rs:662:5
  14:     0x7da8a56fbc25 - core::panicking::panic_nounwind_fmt::runtime::h1c669551f619867f
                               at /rustc/f6e511eec7342f59a25f7c0534f1dbea00d01b14/library/core/src/panicking.rs:112:18
  15:     0x7da8a56fbc25 - core::panicking::panic_nounwind_fmt::hc0ae93930ea8f76c
                               at /rustc/f6e511eec7342f59a25f7c0534f1dbea00d01b14/library/core/src/panicking.rs:122:5
  16:     0x7da8a56fbcb2 - core::panicking::panic_nounwind::h9f485ff9b02bac75
                               at /rustc/f6e511eec7342f59a25f7c0534f1dbea00d01b14/library/core/src/panicking.rs:221:5
  17:     0x7da8a56fbed6 - core::panicking::panic_cannot_unwind::hea865182d7ce50af
                               at /rustc/f6e511eec7342f59a25f7c0534f1dbea00d01b14/library/core/src/panicking.rs:310:5
  18:     0x7da8a578462b - C_CloseAllSessions
                               at /home/.../native-pkcs11/native-pkcs11/src/lib.rs:83:9
  19:     0x7da8a6ec3557 - pkcs11_slot_unref
                               at /build/libp11-qxEbHs/libp11-0.4.12/src/p11_slot.c:433:2
  20:     0x7da8a6ec4530 - pkcs11_release_slot
                               at /build/libp11-qxEbHs/libp11-0.4.12/src/p11_slot.c:477:3
  21:     0x7da8a6ec4530 - pkcs11_release_all_slots
                               at /build/libp11-qxEbHs/libp11-0.4.12/src/p11_slot.c:464:3
  22:     0x7da8a6ec59fd - PKCS11_release_all_slots
                               at /build/libp11-qxEbHs/libp11-0.4.12/src/p11_front.c:111:2
  23:     0x7da8a6ec59fd - ctx_finish
                               at /build/libp11-qxEbHs/libp11-0.4.12/src/eng_back.c:352:4
  24:     0x7da8a6ec59fd - engine_finish
                               at /build/libp11-qxEbHs/libp11-0.4.12/src/eng_front.c:163:8
  25:     0x7da8a69d0c00 - engine_unlocked_finish
                               at /usr/src/openssl-3.0.13-0ubuntu3.4/build_shared/../crypto/engine/eng_init.c:64:21
  26:     0x7da8a69d327e - int_cleanup_cb_doall
                               at /usr/src/openssl-3.0.13-0ubuntu3.4/build_shared/../crypto/engine/eng_table.c:183:9
  27:     0x7da8a69d327e - int_cleanup_cb_doall
                               at /usr/src/openssl-3.0.13-0ubuntu3.4/build_shared/../crypto/engine/eng_table.c:177:13
  28:     0x7da8a6a1db75 - doall_util_fn
                               at /usr/src/openssl-3.0.13-0ubuntu3.4/build_shared/../crypto/lhash/lhash.c:197:17
  29:     0x7da8a6a1db75 - OPENSSL_LH_doall
                               at /usr/src/openssl-3.0.13-0ubuntu3.4/build_shared/../crypto/lhash/lhash.c:205:5
  30:     0x7da8a69d3676 - lh_ENGINE_PILE_doall
                               at /usr/src/openssl-3.0.13-0ubuntu3.4/build_shared/../crypto/engine/eng_local.h:159:1
  31:     0x7da8a69d3676 - engine_table_cleanup
                               at /usr/src/openssl-3.0.13-0ubuntu3.4/build_shared/../crypto/engine/eng_table.c:192:9
  32:     0x7da8a69d0e92 - engine_cleanup_cb_free
                               at /usr/src/openssl-3.0.13-0ubuntu3.4/build_shared/../crypto/engine/eng_lib.c:169:6
  33:     0x7da8a6b4d629 - OPENSSL_sk_pop_free
                               at /usr/src/openssl-3.0.13-0ubuntu3.4/build_shared/../crypto/stack/stack.c:426:13
  34:     0x7da8a69d12c5 - sk_ENGINE_CLEANUP_ITEM_pop_free
                               at /usr/src/openssl-3.0.13-0ubuntu3.4/build_shared/../crypto/engine/eng_local.h:48:1
  35:     0x7da8a69d12c5 - engine_cleanup_int
                               at /usr/src/openssl-3.0.13-0ubuntu3.4/build_shared/../crypto/engine/eng_lib.c:176:9
  36:     0x7da8a6a22846 - OPENSSL_cleanup
                               at /usr/src/openssl-3.0.13-0ubuntu3.4/build_shared/../crypto/init.c:418:5
  37:     0x7da8a6a22846 - OPENSSL_cleanup
                               at /usr/src/openssl-3.0.13-0ubuntu3.4/build_shared/../crypto/init.c:344:6
  38:     0x7da8a6447a66 - __run_exit_handlers
                               at ./stdlib/exit.c:108:8
  39:     0x7da8a6447bae - __GI_exit
                               at ./stdlib/exit.c:138:3
  40:     0x568103f725ae - main
                               at /usr/src/openssl-3.0.13-0ubuntu3.4/build_shared/../apps/openssl.c:311:5
  41:     0x7da8a642a1ca - __libc_start_call_main
                               at ./csu/../sysdeps/nptl/libc_start_call_main.h:58:16
  42:     0x7da8a642a28b - __libc_start_main_impl
                               at ./csu/../csu/libc-start.c:360:3
  43:     0x568103f72765 - _start
  44:                0x0 - <unknown>
thread caused non-unwinding panic. aborting.
Aborted (core dumped)

The changes were checked with my custom backend using RSA2048 and ECC NIST-P256 keys via OpenSSL.

Copy link

google-cla bot commented Nov 22, 2024

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

"Unsupported attribute type - marking as unavailable"
);
attribute.ulValueLen = CK_UNAVAILABLE_INFORMATION;
// result = Err(Error::AttributeTypeInvalid(attribute.type_));
Copy link
Author

Choose a reason for hiding this comment

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

we should skip unknown attributes otherwise a tool that handles pkcs11 calls silently tries to interpret random values from the template: pkcs11-tool and p11-kit produces weird values for attributes when inspecting with pkcs11-spy.

@@ -21,7 +21,7 @@ use std::{
use x509_cert::der::Decode;

pub type Result<T> = std::result::Result<T, Box<dyn std::error::Error>>;
pub type Digest = [u8; 20];
pub type Digest = [u8; 64];
Copy link
Author

Choose a reason for hiding this comment

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

was small to fit ECC P-256 digest value

AttributeType::Verify => Some(Attribute::Verify(true)),
AttributeType::VerifyRecover => Some(Attribute::VerifyRecover(false)),
AttributeType::Wrap => Some(Attribute::Wrap(false)),
AttributeType::Encrypt => Some(Attribute::Encrypt(false)),
Copy link
Author

Choose a reason for hiding this comment

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

Added some missed attributes with default values that pkcs11-tool/p11-kit tools complains about.

@brandonweeks
Copy link
Member

Thank you for the PR! Just to set expectations, I probably won't have an opportunity to review and test until the new year.

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.

2 participants