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

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

People
Owner: Geoff Thorpe
Requestors: Ian Lister
Cc:
AdminCc:

More about the requestors

Ian Lister

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

New reminder:
Subject:
Owner:
Due:

Dates
Created: Wed Apr 23 09:56:35 2008
Starts: Not set
Started: Not set
Last Contact: Thu Jun 12 06:04:21 2008
Due: Not set
Closed: Tue Apr 29 17:57:12 2014
Updated: Tue Apr 29 17:57:12 2014 by Geoff Thorpe



Subject: [PATCH] Fix for engine cache logic reversal
Date: Tue, 22 Apr 2008 23:03:43 +1000 (EST)
To: rt@openssl.org
From: Ian Lister <openssl-rt@lister.dnsalias.net>
Download (untitled) / with headers
text/plain 1.5k
Hi,

It appears the logic in OpenSSL 0.9.8 for caching the default engine for a
given algorithm is inverted. Specifically, the "uptodate" flag in
crypto/engine/eng_table.c is written to in three places with the opposite
sense to what its name and comment implies. It is read from and written to
once elsewhere with the correct sense.

The effect is that loaded engines are not used unless specifically
requested, contrary to the behaviour described under "Automatically using
builtin ENGINE implementations" in the documentation at:

http://www.openssl.org/docs/crypto/engine.html#Application_requirements

I believe the code was written correctly in the original revision 1.1
(checkin 5270) and is correct in all releases of OpenSSL 0.9.7 but was
incorrectly "fixed" in revision 1.6 (checkin 12358) and shipped broken in
all releases of OpenSSL 0.9.8. The attached patch restores the original
behaviour.

The symptom that led me to this was that OpenSSH 5.0p1 (and earlier) was
not getting any benefit of hardware acceleration when running on a VIA C7
(with its Padlock hardware) and FreeBSD 7.0 (with OpenSSL 0.9.8e). The
same machine had previously seen accelerated encryption when running
FreeBSD 6.2 (with OpenSSL 0.9.7e-p1). Modifying OpenSSH to add a call to
ENGINE_set_default_ciphers to explicitly request the Padlock engine be
used (via ENGINE_set_default_ciphers) worked around the issue. Applying
the attached patch fixes it directly. For more background see the bug
report at:

https://bugs.launchpad.net/bugs/119295

Thanks also to John Steele Scott for his work on the issue.

Regards,

Ian

Message body is not shown because sender requested not to inline it.

Download (untitled) / with headers
text/plain 321b
Nice analysis Ian and John, thanks for digging in to this. I agree with
what you've determined, though I think there was a missing 'uptodate'
line from the code too. I'm attaching a diff that matches yours but has
this extra line. Can you please confirm that this still gives you the
behaviour you're expecting? Thanks.

Download diff.diff
text/x-patch 1.1k
Index: crypto/engine/eng_table.c
===================================================================
RCS file: /v/openssl/cvs/openssl/crypto/engine/eng_table.c,v
retrieving revision 1.6.2.1
diff -u -r1.6.2.1 eng_table.c
--- crypto/engine/eng_table.c 6 Sep 2007 12:43:49 -0000 1.6.2.1
+++ crypto/engine/eng_table.c 27 Apr 2008 19:27:52 -0000
@@ -135,7 +135,7 @@
{
fnd = OPENSSL_malloc(sizeof(ENGINE_PILE));
if(!fnd) goto end;
- fnd->uptodate = 0;
+ fnd->uptodate = 1;
fnd->nid = *nids;
fnd->sk = sk_ENGINE_new_null();
if(!fnd->sk)
@@ -152,7 +152,7 @@
if(!sk_ENGINE_push(fnd->sk, e))
goto end;
/* "touch" this ENGINE_PILE */
- fnd->uptodate = 1;
+ fnd->uptodate = 0;
if(setdefault)
{
if(!engine_unlocked_init(e))
@@ -164,6 +164,7 @@
if(fnd->funct)
engine_unlocked_finish(fnd->funct, 0);
fnd->funct = e;
+ fnd->uptodate = 1;
}
nids++;
}
@@ -179,8 +180,7 @@
while((n = sk_ENGINE_find(pile->sk, e)) >= 0)
{
(void)sk_ENGINE_delete(pile->sk, n);
- /* "touch" this ENGINE_CIPHER */
- pile->uptodate = 1;
+ pile->uptodate = 0;
}
if(pile->funct == e)
{
CC: openssl-dev@openssl.org
Subject: Re: [openssl.org #1668] [PATCH] Fix for engine cache logic reversal
Date: Mon, 28 Apr 2008 07:14:48 +1000 (EST)
To: Geoff Thorpe via RT <rt@openssl.org>
From: Ian Lister <openssl-rt@lister.dnsalias.net>
Download (untitled) / with headers
text/plain 429b
Thanks Geoff. Your change looks good.

Ian

On Sun, 27 Apr 2008, Geoff Thorpe via RT wrote:
Show quoted text
> Nice analysis Ian and John, thanks for digging in to this. I agree with
> what you've determined, though I think there was a missing 'uptodate'
> line from the code too. I'm attaching a diff that matches yours but has
> this extra line. Can you please confirm that this still gives you the
> behaviour you're expecting? Thanks.
>
>
>
Subject: Re: [openssl.org #1668] [PATCH] Fix for engine cache logic reversal
Date: Thu, 12 Jun 2008 12:34:03 +0800
To: rt@openssl.org
From: Craig Ringer <craig@postnewspapers.com.au>
This patch appears to have been applied in OpenSSL 0.9.8h.

--
Craig Ringer
This change is already in, and has been for some time. Will close this ticket.
-- 
Geoff Thorpe, RT/openssl.org