Fix SRP #5925

mattcaswell · 2018-04-10T15:31:23Z

Historically we used to implement standalone base64 code for SRP. This
was replaced by commit 3d3f21a with the standard base64 processing code.

SRP has been broken since that time. The immediate issue is that the code was changed to use the low level EVP_EncodeBlock() and EVP_DecodeBlock() functions. Since these are low level functions they do not handle padding. This is done in the higher level EVP_(Encode|Decode)(Update|Final)() functions. This means that when decoding data the binary value does not match the originally encoded value causing SRP connection failures.

This problem caused me to take a deeper look at the SRP base64 code and revealed some more fundamental problems. The SRP base64 code was designed to be compatible with other SRP
libraries (notably libsrp, but also others) that use a variant of standard
base64. Specifically a different alphabet is used and no padding '='
characters are used. Instead 0 padding is added to the front of the string.
By changing to standard base64 we change the behaviour of the API which may
impact interoperability. It also means that SRP verifier files created prior
to 1.1.1 would not be readable in 1.1.1 and vice versa.

This PR expands the standard base64 processing code with the capability to be able to read and generate the SRP base64 variant.

The fact that this problem went undetected for so long is indicative of poor (i.e. non-existent) tests in this area. Therefore I have also added some SRP tests which would have caught these issues.

tests are added or updated

Previously they were using EVP_EncodeBlock/EVP_DecodeBlock. These are low level functions that do not handle padding characters. This was causing the SRP code to fail. One side effect of using EVP_EncodeUpdate is that it inserts newlines which is not what we need in SRP so we add a flag to avoid that.

Historically we used to implement standalone base64 code for SRP. This was replaced by commit 3d3f21a with the standard base64 processing code. However, the SRP base64 code was designed to be compatible with other SRP libraries (notably libsrp, but also others) that use a variant of standard base64. Specifically a different alphabet is used and no padding '=' characters are used. Instead 0 padding is added to the front of the string. By changing to standard base64 we change the behaviour of the API which may impact interoperability. It also means that SRP verifier files created prior to 1.1.1 would not be readable in 1.1.1 and vice versa. Instead we expand our standard base64 processing with the capability to be able to read and generate the SRP base64 variant.

kroeckx · 2018-04-10T17:24:24Z

There are at least warnings that you want to fix.

mattcaswell · 2018-04-11T08:41:24Z

Fixup commit pushed to address the warning picked up by Travis. The Appveyor failure appear unrelated.

dot-asm · 2018-04-11T09:57:44Z

Appveyor failure appear unrelated.

It's being addressed in #5907.

dot-asm · 2018-04-11T10:10:35Z

crypto/srp/srp_vfy.c

+    EVP_ENCODE_CTX *ctx;
+    int outl = 0, outl2 = 0;
+    size_t size, padsize;
+    unsigned char *pad = (unsigned char *)"00";


It's possible and it would be only appropriate to declare this const. After all, literals can and customarily do reside in read-only segment.

dot-asm · 2018-04-11T10:19:53Z

crypto/srp/srp_vfy.c

+    size_t size, padsize;
+    unsigned char *pad = (unsigned char *)"00";
+
+    while (*src != '\0' && (*src == ' ' || *src == '\t' || *src == '\n'))


On side note, formally speaking non-zero check is redundant. This means that some compilers might issue warning. Not that we actually honour such warnings, as we do consciously choose to have constant conditions at occasions...

I removed the zero check.

dot-asm · 2018-04-11T10:22:34Z

crypto/srp/srp_vfy.c

+    size = strlen(src);
+    padsize = 4 - (size & 3);
+    if (padsize == 4)
+        padsize = 0;


Why not just reapply mask, i.e. padsize &= 3? [One can as well (0 - size) & 3, but that would be less readable.]

dot-asm · 2018-04-11T10:36:34Z

crypto/srp/srp_vfy.c


-    /* Four bytes in src become three bytes output. */
-    if (size > INT_MAX || (size / 4) * 3 > alen)


So alen is not longer used? Then why pass it at all? All occurrences are sizeof(something_on_stack), 2500, pretty high, but is there guarantee that it won't overflow?

Hmmm...I think this removing this was over-zealous. I have put this test back.

dot-asm · 2018-04-11T10:55:57Z

crypto/srp/srp_vfy.c

+        if ((int)padsize >= outl)
+            return -1;
+        memmove(a, a + padsize, outl - padsize);
+        outl -= padsize;


This is utterly counter-intuitive. I mean the fact that padsize comes out as same value in encoded and decoded. Maybe a commentary would be appropriate...

I added a comment to explain this.

dot-asm · 2018-04-11T11:04:51Z

crypto/srp/srp_vfy.c

+    evp_encode_ctx_set_flags(ctx, EVP_ENCODE_CTX_NO_NEWLINES
+                                  | EVP_ENCODE_CTX_USE_SRP_ALPHABET);
+
+    /* We pad at the front with zero bytes as required */


I'd appreciate more explicit note here. I mean instead of "as required" something like "till length is divisible by 3 so that encoded string needs no ="...

I expanded out the comment.

dot-asm · 2018-04-11T11:32:32Z

crypto/evp/encode.c

+void evp_encode_ctx_set_flags(EVP_ENCODE_CTX *ctx, uint64_t flags)
+{
+    ctx->flags = flags;
+}


It's not impossible to imagine that this might go public at some point, and in such case I'd argue in favour of using types that don't require additional headers such as unsigned int. In other words I'd like to suggest to consider declaring flags as unsigned int, here and in structure...

Changed to unsigned int.

dot-asm · 2018-04-11T11:38:26Z

crypto/evp/encode.c

 void EVP_DecodeInit(EVP_ENCODE_CTX *ctx)
 {
    /* Only ctx->num is used during decoding. */
    ctx->num = 0;
    ctx->length = 0;
    ctx->line_num = 0;
-    ctx->expect_nl = 0;


So flags don't need initialization? If so how come? I mean if you rely on structure being memset to 0, then you wouldn't need any of initializations. But if you zero them explicitly, then flags deserve it as much, don't they?

The structure is zalloc'd so none of these need initialisation. I removed them all.

…functions

mattcaswell · 2018-04-11T18:49:03Z

Fix up commits pushed addressing the feedback so far.

dot-asm · 2018-04-11T19:37:16Z

crypto/srp/srp_vfy.c

+    EVP_ENCODE_CTX *ctx;
+    int outl = 0, outl2 = 0;
+    size_t size, padsize;
+    const unsigned char *pad = (unsigned char *)"00";


Minor nit, (const unsigned char *)"00".

dot-asm · 2018-04-11T20:16:06Z

crypto/srp/srp_vfy.c

+        ++src;
+    size = strlen(src);
+    padsize = 4 - (size & 3);
+    padsize &= 3;

    /* Four bytes in src become three bytes output. */
    if (size > INT_MAX || (size / 4) * 3 > alen)


Hmmm, one can ~~actually~~ wonder if (size/4)*3 is actually accurate. After all, we'll pre-pad the string, so we do need space for ((size + padsize) / 4) * 3.

dot-asm · 2018-04-11T20:22:31Z

crypto/evp/encode.c

-    ctx->num = 0;
-    ctx->length = 0;
-    ctx->line_num = 0;
-    ctx->expect_nl = 0;


While you are at it, would you leave a commentary that everything is zalloc-ed [so that no member zeroing is required]?

Actually, I changed my mind and put it back to zeroing everything explicitly and added a line to additionally zero the flags. The reason is that it occurred to me that some applications might be reusing an EVP_ENCODE_CTX. This would have worked before but not after I removed the explicit zeroing.

…functions

mattcaswell · 2018-04-12T09:02:29Z

New fixup commits pushed addressing latest feedback.

dot-asm · 2018-04-12T10:02:29Z

osx build complains about memory leak...

mattcaswell · 2018-04-12T13:38:38Z

I can't recreate it locally but I think I've figured out the problem. Fixup commit pushed. Let's see if the CIs think I've fixed it too.

mattcaswell · 2018-04-12T14:37:13Z

Travis is now passing.

mattcaswell · 2018-04-13T08:41:36Z

Pushed. Thanks.

Previously they were using EVP_EncodeBlock/EVP_DecodeBlock. These are low level functions that do not handle padding characters. This was causing the SRP code to fail. One side effect of using EVP_EncodeUpdate is that it inserts newlines which is not what we need in SRP so we add a flag to avoid that. Reviewed-by: Andy Polyakov <appro@openssl.org> (Merged from #5925)

Historically we used to implement standalone base64 code for SRP. This was replaced by commit 3d3f21a with the standard base64 processing code. However, the SRP base64 code was designed to be compatible with other SRP libraries (notably libsrp, but also others) that use a variant of standard base64. Specifically a different alphabet is used and no padding '=' characters are used. Instead 0 padding is added to the front of the string. By changing to standard base64 we change the behaviour of the API which may impact interoperability. It also means that SRP verifier files created prior to 1.1.1 would not be readable in 1.1.1 and vice versa. Instead we expand our standard base64 processing with the capability to be able to read and generate the SRP base64 variant. Reviewed-by: Andy Polyakov <appro@openssl.org> (Merged from #5925)

Reviewed-by: Andy Polyakov <appro@openssl.org> (Merged from #5925)

mattcaswell added 3 commits April 9, 2018 15:09

Add a test for SRP

72c4d12

fixup! Add a test for SRP

e437338

dot-asm reviewed Apr 11, 2018

View reviewed changes

mattcaswell added 2 commits April 11, 2018 19:46

fixup! Change SRP functions to use EVP_EncodeUpdate/EVP_DecodeUpdate …

75fd48e

…functions

fixup! Add support for the SRP base64 alphabet

3d6aa5d

dot-asm reviewed Apr 11, 2018

View reviewed changes

mattcaswell added 2 commits April 12, 2018 09:59

fixup! Change SRP functions to use EVP_EncodeUpdate/EVP_DecodeUpdate …

96a206f

…functions

fixup! Add support for the SRP base64 alphabet

14aeaa8

fixup! Add a test for SRP

96ef8ca

dot-asm approved these changes Apr 12, 2018

View reviewed changes

mattcaswell closed this Apr 13, 2018

levitte pushed a commit that referenced this pull request Apr 13, 2018

Add a test for SRP

76fd7a1

Reviewed-by: Andy Polyakov <appro@openssl.org> (Merged from #5925)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix SRP #5925

Fix SRP #5925

mattcaswell commented Apr 10, 2018

kroeckx commented Apr 10, 2018

mattcaswell commented Apr 11, 2018

dot-asm commented Apr 11, 2018

dot-asm Apr 11, 2018

mattcaswell Apr 11, 2018

dot-asm Apr 11, 2018

mattcaswell Apr 11, 2018

dot-asm Apr 11, 2018

mattcaswell Apr 11, 2018

dot-asm Apr 11, 2018

mattcaswell Apr 11, 2018

dot-asm Apr 11, 2018

mattcaswell Apr 11, 2018

dot-asm Apr 11, 2018

mattcaswell Apr 11, 2018

dot-asm Apr 11, 2018

mattcaswell Apr 11, 2018

dot-asm Apr 11, 2018

mattcaswell Apr 11, 2018

mattcaswell commented Apr 11, 2018

dot-asm Apr 11, 2018

mattcaswell Apr 12, 2018

dot-asm Apr 11, 2018 •

edited

mattcaswell Apr 12, 2018

dot-asm Apr 11, 2018

mattcaswell Apr 12, 2018

mattcaswell commented Apr 12, 2018

dot-asm commented Apr 12, 2018

mattcaswell commented Apr 12, 2018

mattcaswell commented Apr 12, 2018

mattcaswell commented Apr 13, 2018


		/* Four bytes in src become three bytes output. */
		if (size > INT_MAX \|\| (size / 4) * 3 > alen)

Fix SRP #5925

Fix SRP #5925

Conversation

mattcaswell commented Apr 10, 2018

kroeckx commented Apr 10, 2018

mattcaswell commented Apr 11, 2018

dot-asm commented Apr 11, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mattcaswell commented Apr 11, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dot-asm Apr 11, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mattcaswell commented Apr 12, 2018

dot-asm commented Apr 12, 2018

mattcaswell commented Apr 12, 2018

mattcaswell commented Apr 12, 2018

mattcaswell commented Apr 13, 2018

dot-asm Apr 11, 2018 •

edited