Fossil SCM
Revert the previous change after thinking more about it. Login cards in the sync protocol have the following format: login userid nonce signature Nonce is SHA-1 of the message that follows this line, signature is SHA-1 of the concatenation of the nonce and user's shared secret. The successful timing attack can reveal only signature for this particular packet due to nonce. However, as nonce is known to the attacker, it's theoretically possible for them to bruteforce the shared secret_offline_. The whole scenario sounds highly improbable, but using constant-time comparison function for such things by default is a good practice.
Commit
13a9a1244c8c2b420320a569a9e6363dddf2a8f8
Parent
48bcfbd47bf51d4…
2 files changed
+26
+2
-2
+26
| --- src/blob.c | ||
| +++ src/blob.c | ||
| @@ -311,10 +311,36 @@ | ||
| 311 | 311 | sz = szA<szB ? szA : szB; |
| 312 | 312 | rc = memcmp(blob_buffer(pA), blob_buffer(pB), sz); |
| 313 | 313 | if( rc==0 ){ |
| 314 | 314 | rc = szA - szB; |
| 315 | 315 | } |
| 316 | + return rc; | |
| 317 | +} | |
| 318 | + | |
| 319 | +/* | |
| 320 | +** Compare two blobs in constant time and return zero if they are equal. | |
| 321 | +** Constant time comparison only applies for blobs of the same length. | |
| 322 | +** If lengths are different, immediately returns 1. | |
| 323 | +*/ | |
| 324 | +int blob_constant_time_eq(Blob *pA, Blob *pB){ | |
| 325 | + int szA, szB, i; | |
| 326 | + unsigned char *buf1, *buf2; | |
| 327 | + unsigned char rc = 0; | |
| 328 | + | |
| 329 | + blob_is_init(pA); | |
| 330 | + blob_is_init(pB); | |
| 331 | + szA = blob_size(pA); | |
| 332 | + szB = blob_size(pB); | |
| 333 | + if( szA!=szB || szA==0 ) return 1; | |
| 334 | + | |
| 335 | + buf1 = blob_buffer(pA); | |
| 336 | + buf2 = blob_buffer(pB); | |
| 337 | + | |
| 338 | + for( i=0; i<szA; i++ ){ | |
| 339 | + rc = rc | (buf1[i] ^ buf2[i]); | |
| 340 | + } | |
| 341 | + | |
| 316 | 342 | return rc; |
| 317 | 343 | } |
| 318 | 344 | |
| 319 | 345 | /* |
| 320 | 346 | ** Compare a blob to a string. Return TRUE if they are equal. |
| 321 | 347 |
| --- src/blob.c | |
| +++ src/blob.c | |
| @@ -311,10 +311,36 @@ | |
| 311 | sz = szA<szB ? szA : szB; |
| 312 | rc = memcmp(blob_buffer(pA), blob_buffer(pB), sz); |
| 313 | if( rc==0 ){ |
| 314 | rc = szA - szB; |
| 315 | } |
| 316 | return rc; |
| 317 | } |
| 318 | |
| 319 | /* |
| 320 | ** Compare a blob to a string. Return TRUE if they are equal. |
| 321 |
| --- src/blob.c | |
| +++ src/blob.c | |
| @@ -311,10 +311,36 @@ | |
| 311 | sz = szA<szB ? szA : szB; |
| 312 | rc = memcmp(blob_buffer(pA), blob_buffer(pB), sz); |
| 313 | if( rc==0 ){ |
| 314 | rc = szA - szB; |
| 315 | } |
| 316 | return rc; |
| 317 | } |
| 318 | |
| 319 | /* |
| 320 | ** Compare two blobs in constant time and return zero if they are equal. |
| 321 | ** Constant time comparison only applies for blobs of the same length. |
| 322 | ** If lengths are different, immediately returns 1. |
| 323 | */ |
| 324 | int blob_constant_time_eq(Blob *pA, Blob *pB){ |
| 325 | int szA, szB, i; |
| 326 | unsigned char *buf1, *buf2; |
| 327 | unsigned char rc = 0; |
| 328 | |
| 329 | blob_is_init(pA); |
| 330 | blob_is_init(pB); |
| 331 | szA = blob_size(pA); |
| 332 | szB = blob_size(pB); |
| 333 | if( szA!=szB || szA==0 ) return 1; |
| 334 | |
| 335 | buf1 = blob_buffer(pA); |
| 336 | buf2 = blob_buffer(pB); |
| 337 | |
| 338 | for( i=0; i<szA; i++ ){ |
| 339 | rc = rc | (buf1[i] ^ buf2[i]); |
| 340 | } |
| 341 | |
| 342 | return rc; |
| 343 | } |
| 344 | |
| 345 | /* |
| 346 | ** Compare a blob to a string. Return TRUE if they are equal. |
| 347 |
+2
-2
| --- src/xfer.c | ||
| +++ src/xfer.c | ||
| @@ -573,11 +573,11 @@ | ||
| 573 | 573 | blob_zero(&combined); |
| 574 | 574 | blob_copy(&combined, pNonce); |
| 575 | 575 | blob_append(&combined, blob_buffer(&pw), szPw); |
| 576 | 576 | sha1sum_blob(&combined, &hash); |
| 577 | 577 | assert( blob_size(&hash)==40 ); |
| 578 | - rc = blob_compare(&hash, pSig); | |
| 578 | + rc = blob_constant_time_eq(&hash, pSig); | |
| 579 | 579 | blob_reset(&hash); |
| 580 | 580 | blob_reset(&combined); |
| 581 | 581 | if( rc!=0 && szPw!=40 ){ |
| 582 | 582 | /* If this server stores cleartext passwords and the password did not |
| 583 | 583 | ** match, then perhaps the client is sending SHA1 passwords. Try |
| @@ -588,11 +588,11 @@ | ||
| 588 | 588 | blob_zero(&combined); |
| 589 | 589 | blob_copy(&combined, pNonce); |
| 590 | 590 | blob_append(&combined, zSecret, -1); |
| 591 | 591 | free(zSecret); |
| 592 | 592 | sha1sum_blob(&combined, &hash); |
| 593 | - rc = blob_compare(&hash, pSig); | |
| 593 | + rc = blob_constant_time_eq(&hash, pSig); | |
| 594 | 594 | blob_reset(&hash); |
| 595 | 595 | blob_reset(&combined); |
| 596 | 596 | } |
| 597 | 597 | if( rc==0 ){ |
| 598 | 598 | const char *zCap; |
| 599 | 599 |
| --- src/xfer.c | |
| +++ src/xfer.c | |
| @@ -573,11 +573,11 @@ | |
| 573 | blob_zero(&combined); |
| 574 | blob_copy(&combined, pNonce); |
| 575 | blob_append(&combined, blob_buffer(&pw), szPw); |
| 576 | sha1sum_blob(&combined, &hash); |
| 577 | assert( blob_size(&hash)==40 ); |
| 578 | rc = blob_compare(&hash, pSig); |
| 579 | blob_reset(&hash); |
| 580 | blob_reset(&combined); |
| 581 | if( rc!=0 && szPw!=40 ){ |
| 582 | /* If this server stores cleartext passwords and the password did not |
| 583 | ** match, then perhaps the client is sending SHA1 passwords. Try |
| @@ -588,11 +588,11 @@ | |
| 588 | blob_zero(&combined); |
| 589 | blob_copy(&combined, pNonce); |
| 590 | blob_append(&combined, zSecret, -1); |
| 591 | free(zSecret); |
| 592 | sha1sum_blob(&combined, &hash); |
| 593 | rc = blob_compare(&hash, pSig); |
| 594 | blob_reset(&hash); |
| 595 | blob_reset(&combined); |
| 596 | } |
| 597 | if( rc==0 ){ |
| 598 | const char *zCap; |
| 599 |
| --- src/xfer.c | |
| +++ src/xfer.c | |
| @@ -573,11 +573,11 @@ | |
| 573 | blob_zero(&combined); |
| 574 | blob_copy(&combined, pNonce); |
| 575 | blob_append(&combined, blob_buffer(&pw), szPw); |
| 576 | sha1sum_blob(&combined, &hash); |
| 577 | assert( blob_size(&hash)==40 ); |
| 578 | rc = blob_constant_time_eq(&hash, pSig); |
| 579 | blob_reset(&hash); |
| 580 | blob_reset(&combined); |
| 581 | if( rc!=0 && szPw!=40 ){ |
| 582 | /* If this server stores cleartext passwords and the password did not |
| 583 | ** match, then perhaps the client is sending SHA1 passwords. Try |
| @@ -588,11 +588,11 @@ | |
| 588 | blob_zero(&combined); |
| 589 | blob_copy(&combined, pNonce); |
| 590 | blob_append(&combined, zSecret, -1); |
| 591 | free(zSecret); |
| 592 | sha1sum_blob(&combined, &hash); |
| 593 | rc = blob_constant_time_eq(&hash, pSig); |
| 594 | blob_reset(&hash); |
| 595 | blob_reset(&combined); |
| 596 | } |
| 597 | if( rc==0 ){ |
| 598 | const char *zCap; |
| 599 |