[PATCH] Fix SEGV in ssl_scache_shmcb.c

["Mathihalli, Madhusudan" <ma...@hp.com>]

Hello,
	mod_ssl dumps core when you specify a low cache size (Ex. 10000)
OR in a manner similar to Bug 27751. In both the cases, the problem
arises because of a incorrect/incomplete assumption about the size of
the session data in the cache. The session when stored in the cache can
be a maximum of SSL_SESSION_MAX_DER bytes - however, it's NOT safe to
copy SSL_SESSION_MAX_DER bytes back from the cache when we're trying to
retrieve the session id.

The following patch fixes the assumption by including a new 'size'
variable in the cache to store the correct size of the session data - so
that it can be used for retrieval.

Any comments ?

Thanks
-Madhu and Geoff (Thorpe)



RCS file: /home/cvs/httpd-2.0/modules/ssl/ssl_scache_shmcb.c,v
retrieving revision 1.25
diff -u -r1.25 ssl_scache_shmcb.c
--- ssl_scache_shmcb.c  28 Feb 2004 18:06:35 -0000      1.25
+++ ssl_scache_shmcb.c  10 May 2004 20:57:29 -0000
@@ -169,6 +169,7 @@
     unsigned int offset;
     unsigned char s_id2;
     unsigned char removed;
+    unsigned int size;
 } SHMCBIndex;
 
 /* 
@@ -840,6 +841,10 @@
     unsigned int dest_offset,
     unsigned char *src, unsigned int src_len)
 {
+    /* Cover the case that src_len > buf_size */
+    if (src_len > buf_size)
+        src_len = buf_size;
+
     /* Can it be copied all in one go? */
     if (dest_offset + src_len < buf_size)
         /* yes */
@@ -863,6 +868,10 @@
     unsigned int src_offset,
     unsigned int src_len)
 {
+    /* Cover the case that src_len > buf_size */
+    if (src_len > buf_size)
+        src_len = buf_size;
+
     /* Can it be copied all in one go? */
     if (src_offset + src_len < buf_size)
         /* yes */
@@ -1141,6 +1150,7 @@
     shmcb_safe_clear(idx, sizeof(SHMCBIndex));
     shmcb_set_safe_time(&(idx->expires), expiry_time);
     shmcb_set_safe_uint(&(idx->offset), new_offset);
+    shmcb_set_safe_uint(&(idx->size), encoded_len);
 
     /* idx->removed = (unsigned char)0; */ /* Not needed given the
memset above. */
     idx->s_id2 = session_id[1];
@@ -1210,6 +1220,7 @@
             (shmcb_get_safe_time(&(idx->expires)) > now)) {
             unsigned int session_id_length;
             unsigned char *session_id;
+            unsigned int encoded_len =
shmcb_get_safe_uint(&(idx->size));
 
             ap_log_error(APLOG_MARK, APLOG_DEBUG, 0, s,
                          "at index %u, found possible session match",
@@ -1217,9 +1228,9 @@
             shmcb_cyclic_cton_memcpy(header->cache_data_size,
                                      tempasn, cache->data,
 
shmcb_get_safe_uint(&(idx->offset)),
-                                     SSL_SESSION_MAX_DER);
+                                     encoded_len);
             ptr = tempasn;
-            pSession = d2i_SSL_SESSION(NULL, &ptr,
SSL_SESSION_MAX_DER);
+            pSession = d2i_SSL_SESSION(NULL, &ptr, encoded_len);
             session_id_length =
SSL_SESSION_get_session_id_length(pSession);
             session_id = SSL_SESSION_get_session_id(pSession);

Re: [PATCH] Fix SEGV in ssl_scache_shmcb.c

[Geoff Thorpe <ge...@geoffthorpe.net>]

Hi all,

On May 10, 2004 05:04 pm, Mathihalli, Madhusudan wrote:
> 	mod_ssl dumps core when you specify a low cache size (Ex. 10000)
> OR in a manner similar to Bug 27751. In both the cases, the problem
> arises because of a incorrect/incomplete assumption about the size of
> the session data in the cache. The session when stored in the cache can
> be a maximum of SSL_SESSION_MAX_DER bytes - however, it's NOT safe to
> copy SSL_SESSION_MAX_DER bytes back from the cache when we're trying to
> retrieve the session id.
>
> The following patch fixes the assumption by including a new 'size'
> variable in the cache to store the correct size of the session data -
> so that it can be used for retrieval.
>
> Any comments ?

Just one :-) I hadn't been particularly clear about something so wires may 
have got crossed, there is a second patch lurking around and it's purpose 
is overlapped with the one you posted. The patch you sent reduces the 
memcpy() overhead to the minimum required whereas previously it was 
pegged at the maximum possible. The cost for that is the addition of 
another member variable in the index structure. However the use of 
"maximal" memcpy over "minimum" memcpy was not the bug, just an 
inelegance of the code. The real bug was that no check was being made 
that the size of the desired memcpy was less than the size of the 
(sub-)cache, no matter whether it was maximal or minimal! :-) I think the 
bug would have been triggered by maximal and minimal scenarios, provided 
you used small enough cache sizes (less than 256kb) and waited long 
enough.

That second patch is attached to this mail - it is the necessary fix to 
the bug. The other patch is a slight improvement in efficiency (and code 
quality) and would also be useful if it's considered solid enough, but it 
should be independent of the fix.

Cheers,
Geoff
-- 
Geoff Thorpe
geoff@geoffthorpe.net
http://www.geoffthorpe.net/