[openssl-dev] [openssl.org #4175] Add new macro or PKCS7 flag to disable the check for both data and content

2016-06-23 Thread Rich Salz via RT 
fixed some time ago.,

-- 
Ticket here: http://rt.openssl.org/Ticket/Display.html?id=4175
Please log in as guest with password guest if prompted

-- 
openssl-dev mailing list
To unsubscribe: https://mta.openssl.org/mailman/listinfo/openssl-dev

Re: [openssl-dev] [openssl.org #4175] Add new macro or PKCS7 flag to disable the check for both data and content

2016-02-17 Thread Salz, Rich via RT 

> If you say that removing the #ifdef instead of removing the whole code block
> that it contained was a mistake, then I shall take you at your word and 
> refrain
> from harping on *too* much about how naughty it was to have a functional
> change hidden away in a commit which simply entitled itself "Memory leak
> fixes", without even any acknowledgement of the change in the body of the
> commit comment :)

Feel free to dock my pay :)

Looks good.

-- 
Ticket here: http://rt.openssl.org/Ticket/Display.html?id=4175
Please log in as guest with password guest if prompted

-- 
openssl-dev mailing list
To unsubscribe: https://mta.openssl.org/mailman/listinfo/openssl-dev

Re: [openssl-dev] [openssl.org #4175] Add new macro or PKCS7 flag to disable the check for both data and content

2016-02-17 Thread David Woodhouse 
On Tue, 2015-12-08 at 12:56 +, Salz, Rich via RT wrote:
> I think that instead of the #ifdef being removed, the if() test
> should be removed. This was my mistake.

Like this, then... 

https://github.com/openssl/openssl/pull/694 for HEAD
https://github.com/openssl/openssl/pull/695 for 1.0.2

If you say that removing the #ifdef instead of removing the whole code
block that it contained was a mistake, then I shall take you at your
word and refrain from harping on *too* much about how naughty it was to
have a functional change hidden away in a commit which simply entitled
itself "Memory leak fixes", without even any acknowledgement of the
change in the body of the commit comment :)

-- 
David WoodhouseOpen Source Technology Centre
david.woodho...@intel.com  Intel Corporation

From 5e95ba001efb38963a06e1447fde21f06355468d Mon Sep 17 00:00:00 2001
From: David Woodhouse 
Date: Wed, 17 Feb 2016 11:34:14 +
Subject: [PATCH] RT4175: Fix regression using PKCS7_verify() with Authenticode

Authenticode uses an extended PKCS#7 format, where the embedded data are
not directly the data to be verified; instead an Authenticode-specific
data structure (SpcIndirectDataContent) is embedded, which describes
the various files covered by the Authenticode signature.

In this case, we need to allow PKCS7_verify() to be called with external
data even though PKCS7_get_detached() is not true.

This always used to work; there was a "sanity" check for external data
being passed to PKCS7_verify() with a non-detached PKCS#7 signature, but
it was always #ifdef'd out.

It was broken in HEAD by commit 55500ea7c ("GH354: Memory leak fixes") and
in 1.0.2 by cherry-picking that same commit to become c8491de39.
---
 crypto/pkcs7/pk7_smime.c | 6 --
 1 file changed, 6 deletions(-)

diff --git a/crypto/pkcs7/pk7_smime.c b/crypto/pkcs7/pk7_smime.c
index ed5268f..87279a3 100644
--- a/crypto/pkcs7/pk7_smime.c
+++ b/crypto/pkcs7/pk7_smime.c
@@ -279,12 +279,6 @@ int PKCS7_verify(PKCS7 *p7, STACK_OF(X509) *certs, X509_STORE *store,
 return 0;
 }
 
-/* Check for data and content: two sets of data */
-if (!PKCS7_get_detached(p7) && indata) {
-PKCS7err(PKCS7_F_PKCS7_VERIFY, PKCS7_R_CONTENT_AND_DATA_PRESENT);
-return 0;
-}
-
 sinfos = PKCS7_get_signer_info(p7);
 
 if (!sinfos || !sk_PKCS7_SIGNER_INFO_num(sinfos)) {
-- 
2.5.0



smime.p7s
Description: S/MIME cryptographic signature
-- 
openssl-dev mailing list
To unsubscribe: https://mta.openssl.org/mailman/listinfo/openssl-dev

Re: [openssl-dev] [openssl.org #4175] Add new macro or PKCS7 flag to disable the check for both data and content

2016-02-17 Thread David Woodhouse via RT 
On Tue, 2015-12-08 at 12:56 +, Salz, Rich via RT wrote:
> I think that instead of the #ifdef being removed, the if() test
> should be removed. This was my mistake.

Like this, then... 

https://github.com/openssl/openssl/pull/694 for HEAD
https://github.com/openssl/openssl/pull/695 for 1.0.2

If you say that removing the #ifdef instead of removing the whole code
block that it contained was a mistake, then I shall take you at your
word and refrain from harping on *too* much about how naughty it was to
have a functional change hidden away in a commit which simply entitled
itself "Memory leak fixes", without even any acknowledgement of the
change in the body of the commit comment :)

-- 
David WoodhouseOpen Source Technology Centre
david.woodho...@intel.com  Intel Corporation


-- 
Ticket here: http://rt.openssl.org/Ticket/Display.html?id=4175
Please log in as guest with password guest if prompted

>From 5e95ba001efb38963a06e1447fde21f06355468d Mon Sep 17 00:00:00 2001
From: David Woodhouse 
Date: Wed, 17 Feb 2016 11:34:14 +
Subject: [PATCH] RT4175: Fix regression using PKCS7_verify() with Authenticode

Authenticode uses an extended PKCS#7 format, where the embedded data are
not directly the data to be verified; instead an Authenticode-specific
data structure (SpcIndirectDataContent) is embedded, which describes
the various files covered by the Authenticode signature.

In this case, we need to allow PKCS7_verify() to be called with external
data even though PKCS7_get_detached() is not true.

This always used to work; there was a "sanity" check for external data
being passed to PKCS7_verify() with a non-detached PKCS#7 signature, but
it was always #ifdef'd out.

It was broken in HEAD by commit 55500ea7c ("GH354: Memory leak fixes") and
in 1.0.2 by cherry-picking that same commit to become c8491de39.
---
 crypto/pkcs7/pk7_smime.c | 6 --
 1 file changed, 6 deletions(-)

diff --git a/crypto/pkcs7/pk7_smime.c b/crypto/pkcs7/pk7_smime.c
index ed5268f..87279a3 100644
--- a/crypto/pkcs7/pk7_smime.c
+++ b/crypto/pkcs7/pk7_smime.c
@@ -279,12 +279,6 @@ int PKCS7_verify(PKCS7 *p7, STACK_OF(X509) *certs, X509_STORE *store,
 return 0;
 }
 
-/* Check for data and content: two sets of data */
-if (!PKCS7_get_detached(p7) && indata) {
-PKCS7err(PKCS7_F_PKCS7_VERIFY, PKCS7_R_CONTENT_AND_DATA_PRESENT);
-return 0;
-}
-
 sinfos = PKCS7_get_signer_info(p7);
 
 if (!sinfos || !sk_PKCS7_SIGNER_INFO_num(sinfos)) {
-- 
2.5.0



smime.p7s
Description: S/MIME cryptographic signature
-- 
openssl-dev mailing list
To unsubscribe: https://mta.openssl.org/mailman/listinfo/openssl-dev

[openssl-dev] [openssl.org #4175] Add new macro or PKCS7 flag to disable the check for both data and content

2016-02-05 Thread Rich Salz via RT 
can you make a PR (separate from the one you have for UEFI) that does the right
thing? Or attach it to this ticket?
I've kinda lost track :(
--
Rich Salz, OpenSSL dev team; rs...@openssl.org


-- 
Ticket here: http://rt.openssl.org/Ticket/Display.html?id=4175
Please log in as guest with password guest if prompted

-- 
openssl-dev mailing list
To unsubscribe: https://mta.openssl.org/mailman/listinfo/openssl-dev

Re: [openssl-dev] [openssl.org #4175] Add new macro or PKCS7 flag to disable the check for both data and content

2016-02-05 Thread David Woodhouse via RT 
On Fri, 2016-02-05 at 17:31 +, Salz, Rich via RT wrote:
> And update the PR to say that it also closes this ticket :)

Well, it can be a separate PR if the first is already merged...

-- 
dwmw2



-- 
Ticket here: http://rt.openssl.org/Ticket/Display.html?id=4175
Please log in as guest with password guest if prompted



smime.p7s
Description: S/MIME cryptographic signature
-- 
openssl-dev mailing list
To unsubscribe: https://mta.openssl.org/mailman/listinfo/openssl-dev

Re: [openssl-dev] [openssl.org #4175] Add new macro or PKCS7 flag to disable the check for both data and content

2016-02-05 Thread David Woodhouse 
On Fri, 2016-02-05 at 17:31 +, Salz, Rich via RT wrote:
> And update the PR to say that it also closes this ticket :)

Well, it can be a separate PR if the first is already merged...

-- 
dwmw2



smime.p7s
Description: S/MIME cryptographic signature
-- 
openssl-dev mailing list
To unsubscribe: https://mta.openssl.org/mailman/listinfo/openssl-dev

Re: [openssl-dev] [openssl.org #4175] Add new macro or PKCS7 flag to disable the check for both data and content

2016-02-05 Thread Salz, Rich via RT 
And update the PR to say that it also closes this ticket :)



-- 
Ticket here: http://rt.openssl.org/Ticket/Display.html?id=4175
Please log in as guest with password guest if prompted

-- 
openssl-dev mailing list
To unsubscribe: https://mta.openssl.org/mailman/listinfo/openssl-dev

Re: [openssl-dev] [openssl.org #4175] Add new macro or PKCS7 flag to disable the check for both data and content

2016-02-05 Thread David Woodhouse 
On Fri, 2016-02-05 at 17:20 +, Rich Salz via RT wrote:
> can you make a PR (separate from the one you have for UEFI) that does
> the right
> thing? Or attach it to this ticket?
> I've kinda lost track :(

Oops, forgot this one in the set of patches I lined up today. Will add
it. 
-- 
dwmw2



smime.p7s
Description: S/MIME cryptographic signature
-- 
openssl-dev mailing list
To unsubscribe: https://mta.openssl.org/mailman/listinfo/openssl-dev

Re: [openssl-dev] [openssl.org #4175] Add new macro or PKCS7 flag to disable the check for both data and content

2016-02-04 Thread David Woodhouse via RT 
On Tue, 2015-12-08 at 12:56 +, Salz, Rich via RT wrote:
> I think that instead of the #ifdef being removed, the if() test
> should be removed.  
> This was my mistake.

What was the verdict here?

I'm trying to update my builds, as promised this morning. But EDK2 has
updated to 1.0.2e and in doing so, has grown a new patch for this
regression.

So when part of my patch series¹ replaces the existing patch against
1.0.2e with a cleaner patch including all the bug-fixes that have
already gone upstream into HEAD, this needs a "proper" fix...

-- 
David WoodhouseOpen Source Technology Centre
david.woodho...@intel.com  Intel Corporation


¹ http://git.infradead.org/users/dwmw2/edk2.git/commitdiff/16d175c127ac1e


-
http://rt.openssl.org/Ticket/Display.html?id=4175

Please log in as guest with password guest if prompted



smime.p7s
Description: S/MIME cryptographic signature
___
openssl-dev mailing list
To unsubscribe: https://mta.openssl.org/mailman/listinfo/openssl-dev

Re: [openssl-dev] [openssl.org #4175] Add new macro or PKCS7 flag to disable the check for both data and content

2016-02-04 Thread David Woodhouse 
On Tue, 2015-12-08 at 12:56 +, Salz, Rich via RT wrote:
> I think that instead of the #ifdef being removed, the if() test
> should be removed.  
> This was my mistake.

What was the verdict here?

I'm trying to update my builds, as promised this morning. But EDK2 has
updated to 1.0.2e and in doing so, has grown a new patch for this
regression.

So when part of my patch series¹ replaces the existing patch against
1.0.2e with a cleaner patch including all the bug-fixes that have
already gone upstream into HEAD, this needs a "proper" fix...

-- 
David WoodhouseOpen Source Technology Centre
david.woodho...@intel.com  Intel Corporation


¹ http://git.infradead.org/users/dwmw2/edk2.git/commitdiff/16d175c127ac1e


smime.p7s
Description: S/MIME cryptographic signature
___
openssl-dev mailing list
To unsubscribe: https://mta.openssl.org/mailman/listinfo/openssl-dev

[openssl-dev] [openssl.org #4175] Add new macro or PKCS7 flag to disable the check for both data and content

2015-12-08 Thread Long, Qin via RT 
The OpenSSL new release / HEAD updates removed the following comment-out 
statement
in PKCS7_verify() routine, which will return error for one call if both 
embedded-content and
detached data were provided.

#if 0   --> Removed
/*
 * NB: this test commented out because some versions of Netscape
 * illegally include zero length content when signing data.
 */

/* Check for data and content: two sets of data */
if (!PKCS7_get_detached(p7) && indata) {
PKCS7err(PKCS7_F_PKCS7_VERIFY, PKCS7_R_CONTENT_AND_DATA_PRESENT);
return 0;
}
#endif

This update will break some existing Authenticode verification solutions which 
leveraged the
Pkcs7_verify() interface, such as UEFI secure boot, and other open-source 
utilities (e.g. osslsigncode).
The root cause is the Authenticode is one extended PKCS7 format, and its 
verification process
is different (the embedded data is one extended structure 
(SpcIndirectDataContent), and will
not be used directly for signature verification) . The old comment-out in 
PKCS7_verify just helped
to support the Authenticode verification with embedded p7data and user-supplied 
inData
(some extra checking will be handled outside).

It's better to introduce one new macro or new PKCS7 flag to re-enable this 
capability. E.g.

#if !defined(OPENSSL_ALLOW_PKCS7_CONTENT_AND_DATA_PRESENT)

Or
If (!(flags & PKCS7_NO_CHECK_BOTH_DATASET))
...

If two data sets (embedded and detached data) were present, the input data will 
be the default
Input for validation (just as the current logic.), so there should be no risk.


Best Regards & Thanks,
LONG, Qin

___
openssl-bugs-mod mailing list
openssl-bugs-...@openssl.org
https://mta.openssl.org/mailman/listinfo/openssl-bugs-mod___
openssl-dev mailing list
To unsubscribe: https://mta.openssl.org/mailman/listinfo/openssl-dev

Re: [openssl-dev] [openssl.org #4175] Add new macro or PKCS7 flag to disable the check for both data and content

2015-12-08 Thread Salz, Rich via RT 
I think that instead of the #ifdef being removed, the if() test should be 
removed.  
This was my mistake.


___
openssl-dev mailing list
To unsubscribe: https://mta.openssl.org/mailman/listinfo/openssl-dev

Re: [openssl-dev] [openssl.org #4175] Add new macro or PKCS7 flag to disable the check for both data and content

2015-12-08 Thread Long, Qin via RT 
Agree. It will be more straight to remove the if() test here, since the later 
logic will handle the inData directly if this parameter was provided.


> -Original Message-
> From: Salz, Rich via RT [mailto:r...@openssl.org]
> Sent: Tuesday, December 8, 2015 8:56 PM
> To: Long, Qin
> Cc: openssl-dev@openssl.org
> Subject: RE: [openssl-dev] [openssl.org #4175] Add new macro or PKCS7 flag to 
> disable the check for both data and content
> 
> I think that instead of the #ifdef being removed, the if() test should be 
> removed.
> This was my mistake.
> 


___
openssl-dev mailing list
To unsubscribe: https://mta.openssl.org/mailman/listinfo/openssl-dev