Add CMS_verify() method. #1393

sladecek · 2020-12-17T21:06:17Z

Hi,

I need CMS verification for my project.

I found the method in PR #1034 from Jan 2019 by ur0. The PR contained several other methods and waited for tests to be finished. I submit only the CMS_verify() method. A test for both signing and verification was added.

Regards

Ladislav

sfackler · 2020-12-18T01:28:03Z

openssl/src/cms.rs

+    /// [`CMS_verify`]: https://www.openssl.org/docs/manmaster/man3/CMS_verify.html
+    pub fn verify(
+        &mut self,
+        certs: Option<&Stack<X509>>,


This should take a &StackRef, not &Stack.

Changed to &StackRef.

Did a commit not get pushed? I still see &Stack<X509> here.

Sorry. Editing error.

openssl/src/cms.rs

sladecek · 2020-12-19T19:34:54Z

A test fails on one of the architectures. I am working on it.

#0 X509_STORE_get_by_subject (vs=vs@entry=0x7fa5071f6ed0, type=type@entry=1, name=name@entry=0x7fa4a0002220, ret=ret@entry=0x7fa5071f6de0) at x509_lu.c:293
#1 0x0000555b1917440c in X509_STORE_CTX_get1_issuer (issuer=0x7fa5071f6e78, ctx=0x7fa5071f6ed0, x=0x7fa4a0002120) at x509_lu.c:604
#2 0x0000555b191705d9 in X509_verify_cert (ctx=ctx@entry=0x7fa5071f6ed0) at x509_vfy.c:263
#3 0x0000555b1919196f in cms_signerinfo_verify_cert (si=, store=store@entry=0x0, certs=certs@entry=0x7fa4a0004400, crls=crls@entry=0x0, flags=0) at cms_smime.c:282
#4 0x0000555b19192093 in CMS_verify (cms=0x7fa4a0001420, certs=, store=0x0, dcont=0x7fa4a0001cb0, out=0x7fa4a0001dc0, flags=0) at cms_smime.c:344
#5 0x0000555b19072489 in openssl::cms::CmsContentInfo::verify (self=0x7fa5071f7568, certs=..., store=..., indata=..., output_data=..., flags=...) at openssl/src/cms.rs:250
#6 0x0000555b1909811a in openssl::cms::test::cms_sign_verify_error () at openssl/src/cms.rs:420
#7 0x0000555b18fb287a in openssl::cms::test::cms_sign_verify_error::{{closure}} () at openssl/src/cms.rs:394
#8 0x0000555b19035a2e in core::ops::function::FnOnce::call_once<closure,()> () at /rustc/91856ed52c58aa5ba66a015354d1cc69e9779bdf/src/libcore/ops/function.rs:231
#9 0x0000555b190b68bf in {{closure}} () at src/libtest/lib.rs:1497
#10 call_once<closure,()> () at /rustc/91856ed52c58aa5ba66a015354d1cc69e9779bdf/src/libcore/ops/function.rs:231
#11 call_box<(),closure> () at /rustc/91856ed52c58aa5ba66a015354d1cc69e9779bdf/src/liballoc/boxed.rs:749
#12 0x0000555b1922287a in __rust_maybe_catch_panic () at src/libpanic_unwind/lib.rs:87
#13 0x0000555b190d39c8 in try<(),std::panic::AssertUnwindSafe<alloc::boxed::Box<FnBox<()>>>> () at /rustc/91856ed52c58aa5ba66a015354d1cc69e9779bdf/src/libstd/panicking.rs:276
#14 catch_unwind<std::panic::AssertUnwindSafe<alloc::boxed::Box<FnBox<()>>>,()> () at /rustc/91856ed52c58aa5ba66a015354d1cc69e9779bdf/src/libstd/panic.rs:388
#15 {{closure}} () at src/libtest/lib.rs:1452
#16 0x0000555b190af105 in std::sys_common::backtrace::__rust_begin_short_backtrace () at /rustc/91856ed52c58aa5ba66a015354d1cc69e9779bdf/src/libstd/sys_common/backtrace.rs:135
#17 0x0000555b190af8a5 in {{closure}}<closure,()> () at /rustc/91856ed52c58aa5ba66a015354d1cc69e9779bdf/src/libstd/thread/mod.rs:469
#18 call_once<(),closure> () at /rustc/91856ed52c58aa5ba66a015354d1cc69e9779bdf/src/libstd/panic.rs:309
#19 do_call<std::panic::AssertUnwindSafe,()> () at /rustc/91856ed52c58aa5ba66a015354d1cc69e9779bdf/src/libstd/panicking.rs:297
#20 0x0000555b1922287a in __rust_maybe_catch_panic () at src/libpanic_unwind/lib.rs:87
#21 0x0000555b190b69ed in try<(),std::panic::AssertUnwindSafe> () at /rustc/91856ed52c58aa5ba66a015354d1cc69e9779bdf/src/libstd/panicking.rs:276
#22 catch_unwind<std::panic::AssertUnwindSafe,()> () at /rustc/91856ed52c58aa5ba66a015354d1cc69e9779bdf/src/libstd/panic.rs:388
#23 {{closure}}<closure,()> () at /rustc/91856ed52c58aa5ba66a015354d1cc69e9779bdf/src/libstd/thread/mod.rs:468
#24 call_box<(),closure> () at /rustc/91856ed52c58aa5ba66a015354d1cc69e9779bdf/src/liballoc/boxed.rs:749
#25 0x0000555b19221f2e in call_once<(),()> () at /rustc/91856ed52c58aa5ba66a015354d1cc69e9779bdf/src/liballoc/boxed.rs:759
#26 start_thread () at src/libstd/sys_common/thread.rs:14
#27 thread_start () at src/libstd/sys/unix/thread.rs:81
#28 0x00007fa51d5014a4 in ?? ()
#29 0x0000000000000000 in ?? ()

sfackler · 2020-12-25T19:27:46Z

openssl/src/cms.rs

+    /// OpenSSL documentation at [`CMS_verify`]
+    ///
+    /// [`CMS_verify`]: https://www.openssl.org/docs/manmaster/man3/CMS_verify.html
+    #[allow(clippy::redundant_clone)]


What is being redundantly cloned?

*out_data = out_bio.get_buf().to_vec().clone();

I believe that the clone is not redundant, because the source object is not released by rust code but by OpenSSL (see the next comment].

Vec owns its memory. There is no difference between the one created by to_vec and the one created by clone.

OK. I got confused by slice::into_vec(] documentation.

sfackler · 2020-12-25T19:28:11Z

openssl/src/cms.rs

+    ) -> Result<(), ErrorStack> {
+        unsafe {
+            let certs_ptr = certs.map_or(ptr::null_mut(), |p| p.as_ptr());
+            if store.is_none() && !flags.contains(CMSOptions::NO_SIGNER_CERT_VERIFY) {


Does OpenSSL crash if this condition isn't met?

Yes, it does, but not always. Only one of the tests on CI failed (see #1393 (comment) ) probably because input checks are not sufficient in older versions of OpenSSL.

sfackler · 2020-12-25T19:28:51Z

openssl/src/cms.rs

+            ))?;
+
+            if let Some(out_data) = output_data {
+                *out_data = out_bio.get_buf().to_vec().clone();


This clone appears to be unnecessary.

I think than out_bio.get_buf() returns an address in the memory managed by OpenSSL which is released when out_bio is dropped. to_vec() creates a new vector from the existing memory (no copy or clone). Therefore the vector needs to be cloned.

sfackler · 2021-01-01T17:07:13Z

openssl/src/cms.rs

+    /// [`CMS_verify`]: https://www.openssl.org/docs/manmaster/man3/CMS_verify.html
+    pub fn verify(
+        &mut self,
+        certs: Option<&StackRef<X509>>,


The man page for CMS_verify doesn't talk about certs and store being allowed to be null at all - maybe they should just be required?

The man page (https://www.openssl.org/docs/manmaster/man3/CMS_verify.html) mentions a null certs parameter in this sentence:

...An attempt is made to locate all the signing certificate(s), first looking in the certs parameter (if it is not NULL) ...

and there is the option CMS_NO_SIGNER_CERT_VERIFY which disables certificate verification altogether.

Therefore, I think the optional parameters make sense. Nevertheless, the distinction is just a formal one, because the caller can always create an empty vector or an empty store. If you insist, I can change them to required, just let me know.

Ah good find. Keeping certs nullable seems fine, but I'd lean towards requiring store since its nullability isn't mentioned in the docs and at least in 1.0.1 they assume it's present (I'd also hope people aren't using NO_SIGNER_CERT_VERIFY too often!).

sladecek · 2021-01-13T19:08:03Z

Hi,
is there something else for me to correct? I think that all the issues were resolved.

sfackler · 2021-01-15T00:59:42Z

Sorry - github doesn't notify for force pushes :(

It looks like some random commits got somehow pulled into the PR. You may need to do some branch surgery.

sladecek · 2021-01-31T17:53:08Z

Hi,
I deleted the branch and created it anew. Is it OK now?

FelixSchwarz · 2021-09-17T13:18:34Z

@sladecek What happened to this PR? CMS_verify would be really nice to have.

Actually I just saw the diff in this PR + status "closed" so I assumed the PR was merged already and was then wondering why I was unable to call the method... 🤔

sladecek · 2021-09-17T18:47:10Z

I deleted the PR because the maintainer didn't answer for a long time and the branch was outdated.

FelixSchwarz · 2021-09-18T13:40:27Z

Not sure how much I time I have over the next weeks but would having CMS_verify would be really helpful. Would you mind resubmitting the code? I could also take your comit and try to get the code included into rust-openssl if you don't have time to do that.

sladecek · 2021-09-19T12:45:34Z

Hi, the patch is almost a year old, so I need to update it first. Unfortunately, I am leaving for a 14-day vacation. I will look at it in October. Ladislav

…

On Sat, Sep 18, 2021 at 3:40 PM Felix Schwarz ***@***.***> wrote: Not sure how much I time I have over the next weeks but would having CMS_verify would be really helpful. Would you mind resubmitting the code? I could also take your comit and try to get the code included into rust-openssl if you don't have time to do that. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#1393 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ABDGV7J2Y5GOHNXZZAJ4E3TUCSJFNANCNFSM4VAHPL6Q> . Triage notifications on the go with GitHub Mobile for iOS <https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675> or Android <https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>.

-- Ladislav Sládeček

puru1761 · 2022-01-04T20:43:21Z

Hi @sladecek It would be really useful to have this functionality and the APIs included in https://github.com/sfackler/rust-openssl/pull/1034/files for implementing singing of Linux Kernel Modules in rust. Do you know if this is still being worked on?

sladecek · 2022-01-05T18:15:18Z

Here is my PR

#1589

Ladislav

sfackler reviewed Dec 18, 2020

View reviewed changes

openssl/src/cms.rs Outdated Show resolved Hide resolved

sfackler reviewed Dec 18, 2020

View reviewed changes

openssl/src/cms.rs Outdated Show resolved Hide resolved

sfackler reviewed Dec 18, 2020

View reviewed changes

openssl/src/cms.rs Outdated Show resolved Hide resolved

sfackler reviewed Dec 18, 2020

View reviewed changes

openssl/src/cms.rs Outdated Show resolved Hide resolved

sfackler reviewed Dec 25, 2020

View reviewed changes

sfackler reviewed Jan 1, 2021

View reviewed changes

sladecek closed this Jan 19, 2021

Add CMS_verify() method.

402afc5

sladecek reopened this Jan 19, 2021

sladecek closed this Feb 28, 2021

Uh oh!

Add CMS_verify() method. #1393

Add CMS_verify() method. #1393

Uh oh!

Conversation

sladecek commented Dec 17, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

sladecek commented Dec 19, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sladecek commented Jan 13, 2021

Uh oh!

sfackler commented Jan 15, 2021

Uh oh!

sladecek commented Jan 31, 2021

Uh oh!

FelixSchwarz commented Sep 17, 2021

Uh oh!

sladecek commented Sep 17, 2021

Uh oh!

FelixSchwarz commented Sep 18, 2021

Uh oh!

sladecek commented Sep 19, 2021 via email

Uh oh!

puru1761 commented Jan 4, 2022

Uh oh!

sladecek commented Jan 5, 2022

Uh oh!

Uh oh!