Skip Menu |
 
Ticket metadata
The Basics
Id: 2438
Status: resolved
Priority: 0/
Queue: OpenSSL-Bugs

Custom Fields
Milestone: (no value)
Subsystem: (no value)
Severity: (no value)
Broken in: (no value)

People
Owner: Stephen Henson
Requestors: Sebastian Andrzej Siewior
Cc:
AdminCc:

More about the requestors

Sebastian Andrzej Siewior

Comments about this user: No comment entered about this user
Groups this user belongs to
  • Everyone
  • Unprivileged

New reminder:
Subject:
Owner:
Due:

Dates
Created: Sun Jan 23 20:16:44 2011
Starts: Not set
Started: Not set
Last Contact: Wed Jul 02 02:04:52 2014
Due: Not set
Closed: Wed Jul 02 02:04:52 2014
Updated: Wed Jul 02 02:04:52 2014 by Stephen Henson



Subject: [PATCH] ecrypto/ecdsa: fix a zero change in the test suite
Date: Sun, 23 Jan 2011 21:19:08 +0530
To: rt@openssl.org
From: Sebastian Andrzej Siewior <openssl-dev@ml.breakpoint.cc>
Download (untitled) / with headers
text/plain 4.2k
At the end of the testsuite in test_builtin() happens the following:
- a previously created signature gets modified at a random spot
- this signaure is compared against the data which was used to create the
signature.

Now, in theory this should always fail in reality is passed sometimes. The
modifcation algorith did the following:
Show quoted text
| offset = sig[10] % 66;
| dirt = sig[11];
| dirt = dirt ? dirt : 1;
| sig[offset] ^= dirt;

If sig[10] is 0xa7 and sig[11] is 0x9e the last line envolves to:
Show quoted text
| sig[35] ^= 0x9e;

The signature consists of to BIGNUMs encoded as ASN1 string. sig[34] and
sig[35] is the begin of the second and last number. sig[35] contains the
length of this number and its content is 0x1e. Now, 0x9e ^ 0x1e = 0x80
and this is a special value. It means that the length of the value is
"infinite" i.e. everything until the end of the stream. So the ASN1 parser
considers the remaining data as the last element. Since there is nothing
after it, it succeeds. This random modification was a zero change.

This change ensures that something like this does not happen again. If we
do a zero change by accident (R and S are unchanged) we make a third
change and hope that something will change now.

This bug has been reported as a Debian #440538 against 0.9.8e. I could
reproduce it on current 0.9 series, 1.0 looks affected as well.

Signed-off-by: Sebastian Andrzej Siewior <sebastian@breakpoint.cc>
---
crypto/ecdsa/ecdsatest.c | 87 +++++++++++++++++++++++++++++++++++++++++++---
1 files changed, 82 insertions(+), 5 deletions(-)

diff --git a/crypto/ecdsa/ecdsatest.c b/crypto/ecdsa/ecdsatest.c
index aa4e148..67db141 100644
--- a/crypto/ecdsa/ecdsatest.c
+++ b/crypto/ecdsa/ecdsatest.c
@@ -281,6 +281,85 @@ x962_err:
return ret;
}

+static int compare_sig(unsigned char *osig, unsigned int sig_len, ECDSA_SIG *old_sig)
+{
+ unsigned char *signature = osig;
+ ECDSA_SIG *new_sig = NULL;
+ char *org_r = NULL, *org_s = NULL;
+ char *new_r = NULL, *new_s = NULL;
+ int ret = -1;
+
+ org_r = BN_bn2hex(old_sig->r);
+ org_s = BN_bn2hex(old_sig->s);
+ if (!org_r || !org_s)
+ goto out;
+
+ new_sig = ECDSA_SIG_new();
+ if (!new_sig)
+ goto out;
+ if (!d2i_ECDSA_SIG(&new_sig, (const unsigned char **)&signature, sig_len))
+ goto out;
+
+ new_r = BN_bn2hex(new_sig->r);
+ new_s = BN_bn2hex(new_sig->s);
+ if (!new_r || !new_s)
+ goto out;
+ if ((!strcmp(org_r, new_r)) &&
+ !strcmp(org_s, new_s))
+ /* the signature did not change */
+ ret = 1;
+ else
+ ret = 0;
+out:
+ if (new_sig)
+ ECDSA_SIG_free(new_sig);
+ if (new_r)
+ OPENSSL_free(new_r);
+ if (new_s)
+ OPENSSL_free(new_s);
+ if (org_r)
+ OPENSSL_free(org_r);
+ if (org_s)
+ OPENSSL_free(org_s);
+ return ret;
+}
+
+static void modify_signature(unsigned char *osig, unsigned int sig_len, BIO *out)
+{
+ unsigned char dirt, offset;
+ unsigned char *signature = osig;
+ ECDSA_SIG *org_sig;
+ int ret;
+
+ org_sig = ECDSA_SIG_new();
+ if (!org_sig)
+ return;
+
+ if (!d2i_ECDSA_SIG(&org_sig, (const unsigned char **)&signature, sig_len))
+ goto out;
+
+ signature = osig;
+ offset = signature[10] % sig_len;
+ dirt = signature[11];
+ dirt = dirt ? dirt : 1;
+ signature[offset] ^= dirt;
+
+ ret = compare_sig(osig, sig_len, org_sig);
+ if (ret <= 0)
+ goto out;
+
+ signature[offset] = ~signature[offset];
+ ret = compare_sig(osig, sig_len, org_sig);
+ if (ret <= 0)
+ goto out;
+ BIO_printf(out, "Failed to modify signature. Tried: %02x => %02x => %02x\n",
+ (unsigned char) (~osig[offset] ^ dirt),
+ (unsigned char)~osig[offset], osig[offset]);
+ BIO_printf(out, "at offset 0x%02x and it was always equal.\n", offset);
+out:
+ ECDSA_SIG_free(org_sig);
+}
+
int test_builtin(BIO *out)
{
EC_builtin_curve *curves = NULL;
@@ -325,8 +404,6 @@ int test_builtin(BIO *out)
/* now create and verify a signature for every curve */
for (n = 0; n < crv_len; n++)
{
- unsigned char dirt, offset;
-
nid = curves[n].nid;
if (nid == NID_ipsec4)
continue;
@@ -416,9 +493,9 @@ int test_builtin(BIO *out)
BIO_printf(out, ".");
(void)BIO_flush(out);
/* modify a single byte of the signature */
- offset = signature[10] % sig_len;
- dirt = signature[11];
- signature[offset] ^= dirt ? dirt : 1;
+
+ modify_signature(signature, sig_len, out);
+
if (ECDSA_verify(0, digest, 20, signature, sig_len, eckey) == 1)
{
BIO_printf(out, " failed\n");
--
1.7.2.3
Download (untitled) / with headers
text/plain 1.7k
Show quoted text
> [openssl-dev@ml.breakpoint.cc - Sun Jan 23 20:16:44 2011]:
>
> At the end of the testsuite in test_builtin() happens the following:
> - a previously created signature gets modified at a random spot
> - this signaure is compared against the data which was used to create
> the
> signature.
>
> Now, in theory this should always fail in reality is passed sometimes.
> The
> modifcation algorith did the following:
> | offset = sig[10] % 66;
> | dirt = sig[11];
> | dirt = dirt ? dirt : 1;
> | sig[offset] ^= dirt;
>
> If sig[10] is 0xa7 and sig[11] is 0x9e the last line envolves to:
> | sig[35] ^= 0x9e;
>
> The signature consists of to BIGNUMs encoded as ASN1 string. sig[34]
> and
> sig[35] is the begin of the second and last number. sig[35] contains
> the
> length of this number and its content is 0x1e. Now, 0x9e ^ 0x1e = 0x80
> and this is a special value. It means that the length of the value is
> "infinite" i.e. everything until the end of the stream. So the ASN1
> parser
> considers the remaining data as the last element. Since there is
> nothing
> after it, it succeeds. This random modification was a zero change.
>
>

There are several bugs here.

The ASN1 parser should reject indefinite length primitive encodings as
that is illegal.

The original modification routine for ECDSA signatures has another
possible flaw: if the ASN1 is modified so it is no longer valid you
could get an ASN1 parser error. That arguably isn't a good thing if you
want to check signature verification failure.

I think the simplest solution is to so the do_sign and do_verify
functions instead which avoid the ASN1 parser totally and then you can
modify a BIGNUM.

Steve.
--
Dr Stephen N. Henson. OpenSSL project core developer.
Commercial tech support now available see: http://www.openssl.org
Download (untitled) / with headers
text/plain 169b
Resolved now, thanks for the report.

Steve.
--
Dr Stephen N. Henson. OpenSSL project core developer.
Commercial tech support now available see: http://www.openssl.org
CC: openssl-dev@ml.breakpoint.cc
Subject: Re: [openssl.org #2438] Resolved: [PATCH] ecrypto/ecdsa: fix a zero change in the test suite
Date: Fri, 2 Mar 2012 08:03:36 +0100
To: Stephen Henson via RT <rt@openssl.org>
From: Sebastian Andrzej Siewior <sebastian@breakpoint.cc>
Download (untitled) / with headers
text/plain 277b
* Stephen Henson via RT | 2012-02-27 17:43:48 [+0100]:

Show quoted text
>According to our records, your request has been resolved. If you have any
>further questions or concerns, please respond to this message.

Is there a commit id or patch somewhere where I can look at the fix?

Sebastian
Download (untitled) / with headers
text/plain 524b
Show quoted text
> [sebastian@breakpoint.cc - Fri Mar 02 08:55:25 2012]:
>
> * Stephen Henson via RT | 2012-02-27 17:43:48 [+0100]:
>
> >According to our records, your request has been resolved. If you have any
> >further questions or concerns, please respond to this message.
>
> Is there a commit id or patch somewhere where I can look at the fix?
>

Yes, see:

http://cvs.openssl.org/chngview?cn=21780

Steve.
--
Dr Stephen N. Henson. OpenSSL project core developer.
Commercial tech support now available see: http://www.openssl.org
CC: openssl-dev@ml.breakpoint.cc, openssl-dev@openssl.org
Subject: Re: [openssl.org #2438] [PATCH] ecrypto/ecdsa: fix a zero change in the test suite
Date: Sun, 4 Mar 2012 10:37:56 +0100
To: Stephen Henson via RT <rt@openssl.org>
From: Sebastian Andrzej Siewior <sebastian@breakpoint.cc>
Download (untitled) / with headers
text/plain 642b
* Stephen Henson via RT | 2012-03-03 14:41:51 [+0100]:

Show quoted text
>> [sebastian@breakpoint.cc - Fri Mar 02 08:55:25 2012]:
>>
>> * Stephen Henson via RT | 2012-02-27 17:43:48 [+0100]:
>>
>> >According to our records, your request has been resolved. If you have any
>> >further questions or concerns, please respond to this message.
>>
>> Is there a commit id or patch somewhere where I can look at the fix?
>>
>
>Yes, see:
>
>http://cvs.openssl.org/chngview?cn=21780
Thanks.

This changes the testcases but the ASN1 parses still accepts idefinite
length prmitive encodings? I remember someone suggested to change this
as well.

Show quoted text
>Steve.

Sebastian
ASN.1 sanity check added. All cases now resolved.

Steve.
-- 
Dr Stephen N. Henson. OpenSSL project core developer.
Commercial tech support now available see: http://www.openssl.org