Skip Menu | | Logout
Logged in as guest
RT for openssl.org
 
 
#1668: [PATCH] Fix for engine cache logic reversal
X  Ticket metadata  
X  The Basics  
Id: 1668
Status: open
Left: 0 min
Priority: 0/0
Queue: OpenSSL-Bugs

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

X  People  
Owner: geoff <geoff@openssl.org>
Requestors: openssl-rt@lister.dnsalias.net
Cc:
AdminCc:

X  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: Not set
Updated: Thu Jun 12 06:04:21 2008 by craig@postnewspapers.com.au

X  Links  
Depends on:
Depended on by:
Parents:
Children:
Refers to:
Referred to by:

X  Attachments  
diff.diff
engine-uptodate.patch

X  More about Ian Lister  
Comments about this user:
No comment entered about this user
This user's 10 highest priority tickets:
Groups this user belongs to:
  • Everyone
  • Unprivileged

X  History Display mode:[Brief headers] [Full headers]
#     Wed Apr 23 09:56:35 2008  openssl-rt@lister.dnsalias.net - Ticket created    
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)
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
Download engine-uptodate.patch
text/plain 801b
#     Sun Apr 27 21:29:42 2008  geoff - Taken    
#     Sun Apr 27 21:33:38 2008  geoff - Correspondence added    
Download (untitled)
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)
{
#     Sun Apr 27 21:33:39 2008  RT_System - Status changed from 'new' to 'open'    
#     Mon Apr 28 01:14:14 2008  openssl-rt@lister.dnsalias.net - Correspondence added    
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)
text/plain 429b
Thanks Geoff. Your change looks good.

Ian

On Sun, 27 Apr 2008, Geoff Thorpe via RT wrote:
> 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.
>
>
>
#     Thu Jun 12 06:04:20 2008  craig@postnewspapers.com.au - Correspondence added    
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>
Download (untitled)
text/plain 77b
This patch appears to have been applied in OpenSSL 0.9.8h.

--
Craig Ringer
»|« RT 3.4.5 Copyright 1996-2005 Best Practical Solutions, LLC.
Time to display: 0.878301