Enclosed you will find an attached changeset, that contains suggested
fix with keys comparison and completely removed additional protection
via crc32.
Tested also on known to me keys with md5 collisions (see below) - it
works.
If someone needs a git version of it:
https://github.com/sebres/nginx/pull/2 [2]
Below you can find a TCL-code to test strings (hex), that produce an md5
collision (with an example with one collision):
https://github.com/sebres/misc/blob/tcl-test-hash-collision/tcl/hash-collision.tcl
[3]
Regards,
sebres.
On 10.09.2015 11:57, Sergey Brester wrote:
> The patch sounds not bad at all, but I would have also removed the
> calculation and verification of crc32... Makes no sense, if either way the
> keys would be compared.
>
> _______________________________________________
> nginx-devel mailing list
> nginx-devel@nginx.org
> http://mailman.nginx.org/mailman/listinfo/nginx-devel [1]
Links:
------
[1] http://mailman.nginx.org/mailman/listinfo/nginx-devel
[2] https://github.com/sebres/nginx/pull/2
[3]
https://github.com/sebres/misc/blob/tcl-test-hash-collision/tcl/hash-collision.tcl
# HG changeset patch
# User sebres <serg.bres...@sebres.de>
# Date 1441885669 -7200
# Thu Sep 10 13:47:49 2015 +0200
# Node ID 4293b50a7977e86c98bd9ae245dc03156a0a8888
# Parent f829cfb5364c0f32eae7a608c18cab1dc9b06a87
always compares keys of cache entry (hash is insufficient secure, so protects in case of hash collision),
see "http://forum.nginx.org/read.php?29,261413,261529"; for a discussion about;
additional protection via crc32 is obsolete and removed now;
diff -r f829cfb5364c -r 4293b50a7977 src/http/ngx_http_cache.h
--- a/src/http/ngx_http_cache.h Thu Sep 03 15:09:21 2015 +0300
+++ b/src/http/ngx_http_cache.h Thu Sep 10 13:47:49 2015 +0200
@@ -64,7 +64,6 @@ typedef struct {
struct ngx_http_cache_s {
ngx_file_t file;
ngx_array_t keys;
- uint32_t crc32;
u_char key[NGX_HTTP_CACHE_KEY_LEN];
u_char main[NGX_HTTP_CACHE_KEY_LEN];
@@ -119,7 +118,6 @@ typedef struct {
time_t valid_sec;
time_t last_modified;
time_t date;
- uint32_t crc32;
u_short valid_msec;
u_short header_start;
u_short body_start;
diff -r f829cfb5364c -r 4293b50a7977 src/http/ngx_http_file_cache.c
--- a/src/http/ngx_http_file_cache.c Thu Sep 03 15:09:21 2015 +0300
+++ b/src/http/ngx_http_file_cache.c Thu Sep 10 13:47:49 2015 +0200
@@ -233,7 +233,6 @@ ngx_http_file_cache_create_key(ngx_http_
len = 0;
- ngx_crc32_init(c->crc32);
ngx_md5_init(&md5);
key = c->keys.elts;
@@ -243,14 +242,12 @@ ngx_http_file_cache_create_key(ngx_http_
len += key[i].len;
- ngx_crc32_update(&c->crc32, key[i].data, key[i].len);
ngx_md5_update(&md5, key[i].data, key[i].len);
}
c->header_start = sizeof(ngx_http_file_cache_header_t)
+ sizeof(ngx_http_file_cache_key) + len + 1;
- ngx_crc32_final(c->crc32);
ngx_md5_final(c->key, &md5);
ngx_memcpy(c->main, c->key, NGX_HTTP_CACHE_KEY_LEN);
@@ -521,9 +518,12 @@ wakeup:
static ngx_int_t
ngx_http_file_cache_read(ngx_http_request_t *r, ngx_http_cache_t *c)
{
+ u_char *p;
time_t now;
ssize_t n;
+ ngx_str_t *key;
ngx_int_t rc;
+ ngx_uint_t i;
ngx_http_file_cache_t *cache;
ngx_http_file_cache_header_t *h;
@@ -547,12 +547,28 @@ ngx_http_file_cache_read(ngx_http_reques
return NGX_DECLINED;
}
- if (h->crc32 != c->crc32) {
+ if (h->header_start != c->header_start) {
ngx_log_error(NGX_LOG_CRIT, r->connection->log, 0,
- "cache file \"%s\" has md5 collision", c->file.name.data);
+ "cache file \"%s\" has hash collision", c->file.name.data);
return NGX_DECLINED;
}
+ p = c->buf->pos + sizeof(ngx_http_file_cache_header_t)
+ + sizeof(ngx_http_file_cache_key);
+
+ /* because any hash is insufficient, check keys are equal also */
+ key = c->keys.elts;
+ for (i = 0; i < c->keys.nelts; i++) {
+ if (ngx_memcmp(p, key[i].data, key[i].len) != 0) {
+ ngx_log_error(NGX_LOG_CRIT, r->connection->log, 0,
+ "cache file \"%s\" has hash collision",
+ c->file.name.data);
+ return NGX_DECLINED;
+ }
+
+ p += key[i].len;
+ }
+
if ((size_t) h->body_start > c->body_start) {
ngx_log_error(NGX_LOG_CRIT, r->connection->log, 0,
"cache file \"%s\" has too long header",
@@ -583,7 +599,6 @@ ngx_http_file_cache_read(ngx_http_reques
c->last_modified = h->last_modified;
c->date = h->date;
c->valid_msec = h->valid_msec;
- c->header_start = h->header_start;
c->body_start = h->body_start;
c->etag.len = h->etag_len;
c->etag.data = h->etag;
@@ -1229,7 +1244,6 @@ ngx_http_file_cache_set_header(ngx_http_
h->valid_sec = c->valid_sec;
h->last_modified = c->last_modified;
h->date = c->date;
- h->crc32 = c->crc32;
h->valid_msec = (u_short) c->valid_msec;
h->header_start = (u_short) c->header_start;
h->body_start = (u_short) c->body_start;
@@ -1468,7 +1482,6 @@ ngx_http_file_cache_update_header(ngx_ht
if (h.version != NGX_HTTP_CACHE_VERSION
|| h.last_modified != c->last_modified
- || h.crc32 != c->crc32
|| h.header_start != c->header_start
|| h.body_start != c->body_start)
{
@@ -1489,7 +1502,6 @@ ngx_http_file_cache_update_header(ngx_ht
h.valid_sec = c->valid_sec;
h.last_modified = c->last_modified;
h.date = c->date;
- h.crc32 = c->crc32;
h.valid_msec = (u_short) c->valid_msec;
h.header_start = (u_short) c->header_start;
h.body_start = (u_short) c->body_start;
_______________________________________________
nginx-devel mailing list
nginx-devel@nginx.org
http://mailman.nginx.org/mailman/listinfo/nginx-devel
