Re: SSL_OP_NETSCAPE_REUSE_CIPHER_CHANGE_BUG, harmless??

2004-09-18 Thread Jacques A. Vidrine
On Fri, Sep 17, 2004 at 12:39:23AM +0200, Frédéric Giudicelli wrote:
 Jacques A. Vidrine wrote:
 Thanks!  What did you use for this test?  If you are using OpenSSL,
 did the client do SSL_get_session and SSL_set_session?  I'm assuming
 that the handshake completed because your second connection used the
 previously generated master_secret.
 
 You're right, when I unset the master key, the connection fails !

Oh good, then the behavior is as expected.

 But there is still a question I wonder about, how come when 
 SSL_OP_NETSCAPE_REUSE_CIPHER_CHANGE_BUG is set, the list of acceptable 
 ciphers is ignored ?
 If I did set ALL:!NULL, do I really want the user to be allowed to 
 specify RSA-NULL as the new cipher ?

Actually, I am not familiar with the history, but I assume that it is
required to work around quirks in some version of Netscape.  Maybe
someone else will be able to tell us more.

Cheers,
-- 
Jacques A Vidrine / NTT/Verio
[EMAIL PROTECTED] / [EMAIL PROTECTED] / [EMAIL PROTECTED]
__
OpenSSL Project http://www.openssl.org
Development Mailing List   [EMAIL PROTECTED]
Automated List Manager   [EMAIL PROTECTED]


Re: SSL_OP_NETSCAPE_REUSE_CIPHER_CHANGE_BUG, harmless??

2004-09-16 Thread Jacques A. Vidrine
On Thu, Sep 16, 2004 at 10:55:46AM +0200, Frédéric Giudicelli wrote:
 Target: openssl-0.9.7d
 
 Hi,
 
 In ssl/ssl.h, SSL_OP_ALL is defined as various bug workarounds that
 should be rather harmless, however it includes the
 SSL_OP_NETSCAPE_REUSE_CIPHER_CHANGE_BUG option, which is rather not a
 harmless option, since it allows a user to change the cipher during a
 session resuming.
 The problem is that it by-passes the verification of acceptable ciphers on
 server side. Therefore, if some one is able to get a valid session id from
 another user (by sniffing or whatever other way), it will be able to
 connect to the server and specify the RSA-NULL cipher, therefore the
 previous session won't be needed, and there won't be any proof that the
 current user is the previous one (ssl session hijacking).

``therefore the previous session won't be needed''?  But the handshake
still must be completed, must it not?  And to do so, the attacker
would need to know the master_secret (for the Finished messages).
I must be missing something.  Would you mind explaining a bit further
for the slow? :-)

Cheers,
-- 
Jacques A Vidrine / NTT/Verio
[EMAIL PROTECTED] / [EMAIL PROTECTED] / [EMAIL PROTECTED]
__
OpenSSL Project http://www.openssl.org
Development Mailing List   [EMAIL PROTECTED]
Automated List Manager   [EMAIL PROTECTED]


Re: SSL_OP_NETSCAPE_REUSE_CIPHER_CHANGE_BUG, harmless??

2004-09-16 Thread Jacques A. Vidrine
On Thu, Sep 16, 2004 at 09:51:24PM +0200, Frédéric Giudicelli wrote:
 Jacques A. Vidrine wrote:
 ``therefore the previous session won't be needed''?  But the handshake
 still must be completed, must it not?  And to do so, the attacker
 would need to know the master_secret (for the Finished messages).
 I must be missing something.  Would you mind explaining a bit further
 for the slow? :-)
 
 Cheers,


 Hi,

 Yes the handshake is completed.
 To resume a session the client set the Session ID field in the Client
 Hello message, and that's it.

Then is there some additional bug?  SSL session resumption requires

C-S:   ClientHello w/session_id
S-C:   ServerHello w/same session_id
ChangeCipherSpec
Finished
C-S:   ChangeCipherSpec
Finished

The session is not resumed before all of these messages have been
processed.  In this scenario, the client (the attacker) would not be
able to produce an acceptable Finished message because it does not have
access to the master_secret.  Even if the Finished message is not
encrypted, the contents are dependent upon the master_secret.  i.e. for
SSLv3 we have

   MD5(master_secret + padding +
 MD5(messages + client + master_secret + padding))
   SHA1(master_secret + padding +
 SHA1(messages + client + master_secret + padding))

 Normally the server is sure that it's the
 real client, because the client will be able to decrypt the datas using
 the symetrical key that was exchanged during the previous handshake.

Hmm, I thought that the server is sure that it's the real client because
only the real client could complete the handshake.

 The problem is that the datas won't be crypted anymore because during
 the Client Hello message the attacker specified a NULL cipher...

 I'm attaching a network dump of both the connections, you will see the
 problem :)

Thanks!  What did you use for this test?  If you are using OpenSSL,
did the client do SSL_get_session and SSL_set_session?  I'm assuming
that the handshake completed because your second connection used the
previously generated master_secret.

 (The resumption you sent)
  New TCP connection #2: 10.0.0.1(40278) - 10.0.0.253(443)
  2 1  0.0005 (0.0005)  CS  Handshake
ClientHello
  Version 3.0
  resume [32]=
6c 98 fa ec 78 cd 3d 8b 4e e9 71 78 b9 61 6d 0d
9d 1e aa 14 6c 48 73 1e 8b 7f 5d a5 61 4e 72 ed
  cipher suites
  SSL_RSA_WITH_NULL_SHA
  compression methods
NULL
  2 2  0.0025 (0.0020)  SC  Handshake
ServerHello
  Version 3.0
  session_id[32]=
6c 98 fa ec 78 cd 3d 8b 4e e9 71 78 b9 61 6d 0d
9d 1e aa 14 6c 48 73 1e 8b 7f 5d a5 61 4e 72 ed
  cipherSuite SSL_RSA_WITH_NULL_SHA
  compressionMethod   NULL
  2 3  0.0025 (0.)  SC  ChangeCipherSpec
  2 4  0.0025 (0.)  SC  Handshake
  2 5  0.0027 (0.0001)  CS  ChangeCipherSpec
  2 6  0.0027 (0.)  CS  Handshake
  2 7  0.0430 (0.0402)  CS  application_data
  2 8  0.0430 (0.)  CS  application_data
  2 9  0.0459 (0.0029)  SC  application_data
  2 10 0.0459 (0.)  SC  application_data
  2 11 0.0461 (0.0001)  SC  Alert
  20.0463 (0.0001)  SC  TCP FIN
  2 12 0.0468 (0.0004)  CS  Alert
  20.0469 (0.)  CS  TCP FIN

If this were a real attack, then the server should have aborted the
message upon receiving message #6, should it not have?

Cheers,
-- 
Jacques A Vidrine / NTT/Verio
[EMAIL PROTECTED] / [EMAIL PROTECTED] / [EMAIL PROTECTED]
__
OpenSSL Project http://www.openssl.org
Development Mailing List   [EMAIL PROTECTED]
Automated List Manager   [EMAIL PROTECTED]


Re: IMPORTANT: please test snapshot openssl-0.9.7-stable-SNAP-20030214.tar.gz

2003-02-19 Thread Jacques A. Vidrine
On Fri, Feb 14, 2003 at 06:08:34PM +0100, Bodo Moeller wrote:
 Please test snapshot openssl-0.9.7-stable-SNAP-20030214.tar.gz
 (or later), which will be available today around 8 p.m. GMT at
 URL: ftp://ftp.openssl.org/snapshot;type=d .
 
 We plan to release version 0.9.7a soon (next week if all goes well).
 OpenSSL 0.9.7a will be a bugfix release based on 0.9.7; thus there
 will be no beta releases.  The snapshot should solve most problems
 that have been reported to [EMAIL PROTECTED]; please test it to help us
 avoid unforeseen problems with the new release.

`make test' completed for openssl-0.9.7-stable-SNAP-20030216 on
FreeBSD 5.0-CURRENT i386, alpha, and sparc64.

Cheers,
-- 
Jacques A. Vidrine [EMAIL PROTECTED]  http://www.celabo.org/
NTT/Verio SME  . FreeBSD UNIX .   Heimdal Kerberos
[EMAIL PROTECTED] .  [EMAIL PROTECTED]  .  [EMAIL PROTECTED]
__
OpenSSL Project http://www.openssl.org
Development Mailing List   [EMAIL PROTECTED]
Automated List Manager   [EMAIL PROTECTED]



Re: [openssl.org #456] openssl 0.9.7, bug in ui_lib.c:general_allocate_string

2003-01-22 Thread Jacques A. Vidrine
On Mon, Jan 13, 2003 at 03:16:46PM +0100, Richard Levitte via RT wrote:

 Hello,

 Thanks for the report.  Unfortunately, your conclusions are
 incorrect.  The functions that you spotted in ui_lib.c return the
 expected values, it's UI_UTIL_read_pw() that interprets those values
 incorrectly.

Ah, I see.  There seems to be a bit of confusion about that.  A quick
survey of consumers of general_allocate_string:

 general_allocate_string
  In reality, returns negative for error

UI_add_input_string
Comment says `Returns the index to the place in the stack or 0 for
error.' -- zero for error
   password_callback
 `ok = 0' -- negative for error
   hwcrhk_get_pass
 ignores return value
   EVP_read_pw_string
 ignores return value
   UI_UTIL_read_pw
 `ok == 0' -- non-zero for error

UI_dup_input_string
Comment says `Same as UI_add_input_string()' -- zero for error

UI_add_verify_string
   password_callback
 `ok = 0' -- negative for error
   EVP_read_pw_string
 ignores return value
   UI_UTIL_read_pw
 `ok == 0' -- non-zero for error

UI_dup_verify_string
No callers

UI_add_info_string
No callers

UI_dup_info_string
   hwcrhk_insert_card
`ok = 0' -- negative for error

UI_add_error_string
   No callers

UI_dup_error_string
   No callers


So yeah, I guess UI_UTIL_read_pw and the comment for
UI_add_input_string et. al. are incorrect.  Previously I stopped
looking after seeing that comment :-)  oops

 I'm committing a change that should fix this.  Please try tomorrows
 snapshot.

 This ticket is now resolved.

Thanks much!  Your fix looks correct to me.
Cheers,
-- 
Jacques A. Vidrine [EMAIL PROTECTED]  http://www.celabo.org/
NTT/Verio SME  . FreeBSD UNIX .   Heimdal Kerberos
[EMAIL PROTECTED] .  [EMAIL PROTECTED]  .  [EMAIL PROTECTED]
__
OpenSSL Project http://www.openssl.org
Development Mailing List   [EMAIL PROTECTED]
Automated List Manager   [EMAIL PROTECTED]



[openssl.org #462] Enable cryptodev engine for /dev/crypto on FreeBSD

2003-01-16 Thread Jacques A. Vidrine via RT

Hello,

The following modifications (or similar) are needed to enable
cryptodev support on FreeBSD (which has imported OpenBSD's
/dev/crypto).  FreeBSD's /dev/crypto is available in 5.0-RELEASE and
in 4-STABLE (and the upcoming 4.8-RELEASE).

Cheers,
-- 
Jacques A. Vidrine [EMAIL PROTECTED]  http://www.celabo.org/
NTT/Verio SME  . FreeBSD UNIX .   Heimdal Kerberos
[EMAIL PROTECTED] .  [EMAIL PROTECTED]  .  [EMAIL PROTECTED]


diff -ru openssl-0.9.7/crypto/engine/eng_all.c openssl-0.9.7/crypto/engine/eng_all.c
--- openssl-0.9.7/crypto/engine/eng_all.c   Tue Oct  1 19:18:47 2002
+++ openssl-0.9.7/crypto/engine/eng_all.c   Mon Jan 13 06:21:33 2003
@@ -95,19 +95,19 @@
 #ifndef OPENSSL_NO_HW_4758_CCA
ENGINE_load_4758cca();
 #endif
-#ifdef __OpenBSD__
+#if defined(__OpenBSD__) || defined(__FreeBSD__)
ENGINE_load_cryptodev();
 #endif
 #endif
}
 
-#ifdef __OpenBSD__
-void ENGINE_setup_openbsd(void) {
-   static int openbsd_default_loaded = 0;
-   if (!openbsd_default_loaded) {
+#if defined(__OpenBSD__) || defined(__FreeBSD__)
+void ENGINE_setup_bsd_cryptodev(void) {
+   static int bsd_cryptodev_default_loaded = 0;
+   if (!bsd_cryptodev_default_loaded) {
ENGINE_load_cryptodev();
ENGINE_register_all_complete();
}
-   openbsd_default_loaded=1;
+   bsd_cryptodev_default_loaded=1;
 }
 #endif
diff -ru openssl-0.9.7/crypto/engine/hw_cryptodev.c 
openssl-0.9.7/crypto/engine/hw_cryptodev.c
--- openssl-0.9.7/crypto/engine/hw_cryptodev.c  Thu Dec  5 04:17:08 2002
+++ openssl-0.9.7/crypto/engine/hw_cryptodev.c  Mon Jan 13 09:29:09 2003
@@ -33,31 +33,28 @@
 #include openssl/engine.h
 #include openssl/evp.h
 
-#ifndef __OpenBSD__
-
-void
-ENGINE_load_cryptodev(void)
-{
-   /* This is a NOP unless __OpenBSD__ is defined */
-   return;
-}
-
-#else /* __OpenBSD__ */
-
-#include sys/types.h
+#if (defined(__unix__) || defined(unix))  !defined(USG)
 #include sys/param.h
+# if (OpenBSD = 200112) || ((__FreeBSD_version = 470101  __FreeBSD_version  
+5) || __FreeBSD_version = 50041)
+# define HAVE_CRYPTODEV
+# endif
+# if (OpenBSD = 200110)
+# define HAVE_SYSLOG_R
+# endif
+#endif
 
-#if OpenBSD  200112
+#ifndef HAVE_CRYPTODEV
 
 void
 ENGINE_load_cryptodev(void)
 {
-   /* This is a NOP unless we have release 3.0 (released december 2001) */
+   /* This is a NOP on platforms without /dev/crypto */
return;
 }
 
-#else /* OpenBSD 3.0 or above */
+#else 
 
+#include sys/types.h
 #include crypto/cryptodev.h
 #include sys/ioctl.h
 #include errno.h
@@ -1032,12 +1029,18 @@
 static int
 cryptodev_ctrl(ENGINE *e, int cmd, long i, void *p, void (*f)())
 {
+#ifdef HAVE_SYSLOG_R
struct syslog_data sd = SYSLOG_DATA_INIT;
+#endif
 
switch (cmd) {
default:
+#ifdef HAVE_SYSLOG_R
syslog_r(LOG_ERR, sd,
cryptodev_ctrl: unknown command %d, cmd);
+#else
+   syslog(LOG_ERR, cryptodev_ctrl: unknown command %d, cmd);
+#endif
break;
}
return (1);
@@ -1064,7 +1067,7 @@
close(fd);
 
if (!ENGINE_set_id(engine, cryptodev) ||
-   !ENGINE_set_name(engine, OpenBSD cryptodev engine) ||
+   !ENGINE_set_name(engine, BSD cryptodev engine) ||
!ENGINE_set_ciphers(engine, cryptodev_engine_ciphers) ||
!ENGINE_set_digests(engine, cryptodev_engine_digests) ||
!ENGINE_set_ctrl_function(engine, cryptodev_ctrl) ||
@@ -1126,5 +1129,4 @@
ERR_clear_error();
 }
 
-#endif /* OpenBSD 3.0 or above */
-#endif /* __OpenBSD__ */
+#endif /* HAVE_CRYPTODEV */
diff -ru openssl-0.9.7/crypto/evp/c_all.c openssl-0.9.7/crypto/evp/c_all.c
--- openssl-0.9.7/crypto/evp/c_all.cTue Oct  1 19:18:55 2002
+++ openssl-0.9.7/crypto/evp/c_all.cThu Jan 16 10:39:37 2003
@@ -73,7 +73,7 @@
{
OpenSSL_add_all_ciphers();
OpenSSL_add_all_digests();
-#ifdef __OpenBSD__
-   ENGINE_setup_openbsd();
+#if defined(__OpenBSD__) || defined(__FreeBSD__)
+   ENGINE_setup_bsd_cryptodev();
 #endif
}

__
OpenSSL Project http://www.openssl.org
Development Mailing List   [EMAIL PROTECTED]
Automated List Manager   [EMAIL PROTECTED]



[openssl.org #456] openssl 0.9.7, bug in ui_lib.c:general_allocate_string

2003-01-13 Thread Jacques A. Vidrine via RT

[ Sorry if you see this twice.  I missed the existence of the RT alias
  before sending this to openssl-dev earlier. ]

Hello,

Regarding openssl 0.9.7:

When using OPENSSL_DES_LIBDES_COMPATIBILITY, I noticed that
`des_read_pw_string' was not functioning.  I tracked this down to a
bug in crypto/ui/ui_lib.c:general_allocate_string().

Callers of general_allocate_string (including ultimately
UI_UTIL_read_pw, which is used to implement des_read_pw_string) expect
it to return 0 for success, or non-zero for failure.  However, the
return code is mishandled here:

164 static int general_allocate_string(UI *ui, const char *prompt,
 [...]
168 int ret = -1;
 [...]
179 ret=sk_UI_STRING_push(ui-strings, s);
180 /* sk_push() returns 0 on error.  Let's addapt that */
181 if (ret = 0) ret--;

sk_UI_STRING_push returns 0 on error, or a positive integer
for success.  Therefore, if sk_UI_STRING_push succeeds,
general_allocate_string returns a positive integer, which does not
match what callers expect.

This is the simple fix I applied locally (note the same issue exists
in general_allocate_boolean).

--- ui_lib.cWed Dec  4 18:04:40 2002
+++ ui_lib.cSun Jan 12 09:04:16 2003
@@ -178,7 +178,7 @@
s-_.string_data.test_buf=test_buf;
ret=sk_UI_STRING_push(ui-strings, s);
/* sk_push() returns 0 on error.  Let's addapt that */
-   if (ret = 0) ret--;
+   ret = (ret == 0) ? -1 : 0;
}
else
free_string(s);
@@ -228,7 +228,7 @@
ret=sk_UI_STRING_push(ui-strings, s);
/* sk_push() returns 0 on error.
   Let's addapt that */
-   if (ret = 0) ret--;
+   ret = (ret == 0) ? -1 : 0;
}
else
free_string(s);


Cheers,
-- 
Jacques A. Vidrine [EMAIL PROTECTED]  http://www.celabo.org/
NTT/Verio SME  . FreeBSD UNIX .   Heimdal Kerberos
[EMAIL PROTECTED] .  [EMAIL PROTECTED]  .  [EMAIL PROTECTED]

__
OpenSSL Project http://www.openssl.org
Development Mailing List   [EMAIL PROTECTED]
Automated List Manager   [EMAIL PROTECTED]



Re: [openssl.org #456] openssl 0.9.7, bug in ui_lib.c:general_allocate_string

2003-01-13 Thread Jacques A. Vidrine via RT

On Mon, Jan 13, 2003 at 03:16:46PM +0100, Richard Levitte via RT wrote:

 Hello,

 Thanks for the report.  Unfortunately, your conclusions are
 incorrect.  The functions that you spotted in ui_lib.c return the
 expected values, it's UI_UTIL_read_pw() that interprets those values
 incorrectly.

Ah, I see.  There seems to be a bit of confusion about that.  A quick
survey of consumers of general_allocate_string:

 general_allocate_string
  In reality, returns negative for error

UI_add_input_string
Comment says `Returns the index to the place in the stack or 0 for
error.' -- zero for error
   password_callback
 `ok = 0' -- negative for error
   hwcrhk_get_pass
 ignores return value
   EVP_read_pw_string
 ignores return value
   UI_UTIL_read_pw
 `ok == 0' -- non-zero for error

UI_dup_input_string
Comment says `Same as UI_add_input_string()' -- zero for error

UI_add_verify_string
   password_callback
 `ok = 0' -- negative for error
   EVP_read_pw_string
 ignores return value
   UI_UTIL_read_pw
 `ok == 0' -- non-zero for error

UI_dup_verify_string
No callers

UI_add_info_string
No callers

UI_dup_info_string
   hwcrhk_insert_card
`ok = 0' -- negative for error

UI_add_error_string
   No callers

UI_dup_error_string
   No callers


So yeah, I guess UI_UTIL_read_pw and the comment for
UI_add_input_string et. al. are incorrect.  Previously I stopped
looking after seeing that comment :-)  oops

 I'm committing a change that should fix this.  Please try tomorrows
 snapshot.

 This ticket is now resolved.

Thanks much!  Your fix looks correct to me.
Cheers,
-- 
Jacques A. Vidrine [EMAIL PROTECTED]  http://www.celabo.org/
NTT/Verio SME  . FreeBSD UNIX .   Heimdal Kerberos
[EMAIL PROTECTED] .  [EMAIL PROTECTED]  .  [EMAIL PROTECTED]

__
OpenSSL Project http://www.openssl.org
Development Mailing List   [EMAIL PROTECTED]
Automated List Manager   [EMAIL PROTECTED]



openssl 0.9.7, bug in ui_lib.c:general_allocate_string

2003-01-12 Thread Jacques A. Vidrine
Hello,

Regarding openssl 0.9.7:

When using OPENSSL_DES_LIBDES_COMPATIBILITY, I noticed that
`des_read_pw_string' was not functioning.  I tracked this down to a
bug in crypto/ui/ui_lib.c:general_allocate_string().

Callers of general_allocate_string (including ultimately
UI_UTIL_read_pw, which is used to implement des_read_pw_string) expect
it to return 0 for success, or non-zero for failure.  However, the
return code is mishandled here:

164 static int general_allocate_string(UI *ui, const char *prompt,
 [...]
168 int ret = -1;
 [...]
179 ret=sk_UI_STRING_push(ui-strings, s);
180 /* sk_push() returns 0 on error.  Let's addapt that */
181 if (ret = 0) ret--;

sk_UI_STRING_push returns 0 on error, or a positive integer
for success.  Therefore, if sk_UI_STRING_push succeeds,
general_allocate_string returns a positive integer, which does not
match what callers expect.

This is the simple fix I applied locally (note the same issue exists
in general_allocate_boolean).

--- ui_lib.cWed Dec  4 18:04:40 2002
+++ ui_lib.cSun Jan 12 09:04:16 2003
@@ -178,7 +178,7 @@
s-_.string_data.test_buf=test_buf;
ret=sk_UI_STRING_push(ui-strings, s);
/* sk_push() returns 0 on error.  Let's addapt that */
-   if (ret = 0) ret--;
+   ret = (ret == 0) ? -1 : 0;
}
else
free_string(s);
@@ -228,7 +228,7 @@
ret=sk_UI_STRING_push(ui-strings, s);
/* sk_push() returns 0 on error.
   Let's addapt that */
-   if (ret = 0) ret--;
+   ret = (ret == 0) ? -1 : 0;
}
else
free_string(s);


Cheers,
-- 
Jacques A. Vidrine [EMAIL PROTECTED]  http://www.celabo.org/
NTT/Verio SME  . FreeBSD UNIX .   Heimdal Kerberos
[EMAIL PROTECTED] .  [EMAIL PROTECTED]  .  [EMAIL PROTECTED]
__
OpenSSL Project http://www.openssl.org
Development Mailing List   [EMAIL PROTECTED]
Automated List Manager   [EMAIL PROTECTED]