code review 9222045: crypto/rsa: check for minimal PKCS#1 v1.5 padding.

crypto/rsa: check for minimal PKCS#1 v1.5 padding.

The PKCS#1 spec requires that the PS padding in an RSA message be at
least 8 bytes long. We were not previously checking this. This isn't
important in the most common situation (session key encryption), but
the impact is unclear in other cases.

This change enforces the specified minimum size.

[agl1, 2013-05-14 21:43:32]

Hello golang-dev@googlegroups.com (cc: golang-dev@googlegroups.com),

I'd like you to review this change to
https://code.google.com/p/go/

[bradfitz, 2013-05-14 22:09:56]

Equal is spelled Eq elsewhere in crypto/subtle. Why inconsistent here?



On Tue, May 14, 2013 at 2:43 PM, <agl@golang.org> wrote:

> Reviewers: golang-dev1,
>
> Message:
> Hello golang-dev@googlegroups.com (cc: golang-dev@googlegroups.com),
>
> I'd like you to review this change to
> https://code.google.com/p/go/
>
>
> Description:
> crypto/rsa: check for minimal PKCS#1 v1.5 padding.
>
> The PKCS#1 spec requires that the PS padding in an RSA message be at
> least 8 bytes long. We were not previously checking this. This isn't
> important in the most common situation (session key encryption), but
> the impact is unclear in other cases.
>
> This change enforces the specified minimum size.
>
> Please review this at
https://codereview.appspot.**com/9222045/<https://codereview.appspot.com/9222...
>
> Affected files:
>   M src/pkg/crypto/rsa/pkcs1v15.go
>   M src/pkg/crypto/rsa/pkcs1v15_**test.go
>   M src/pkg/crypto/subtle/**constant_time.go
>   M src/pkg/crypto/subtle/**constant_time_test.go
>
>
> Index: src/pkg/crypto/rsa/pkcs1v15.go
> ==============================**==============================**=======
> --- a/src/pkg/crypto/rsa/pkcs1v15.**go
> +++ b/src/pkg/crypto/rsa/pkcs1v15.**go
> @@ -124,7 +124,11 @@
>                 lookingForIndex = subtle.ConstantTimeSelect(**equals0, 0,
> lookingForIndex)
>         }
>
> -       valid = firstByteIsZero & secondByteIsTwo & (^lookingForIndex & 1)
> +       // The PS padding must be at least 8 bytes long, and it starts two
> +       // bytes into em.
> +       validPS := subtle.**ConstantTimeLessOrEqual(2+8, index)
> +
> +       valid = firstByteIsZero & secondByteIsTwo & (^lookingForIndex & 1)
> & validPS
>         msg = em[index+1:]
>         return
>  }
> Index: src/pkg/crypto/rsa/pkcs1v15_**test.go
> ==============================**==============================**=======
> --- a/src/pkg/crypto/rsa/pkcs1v15_**test.go
> +++ b/src/pkg/crypto/rsa/pkcs1v15_**test.go
> @@ -197,6 +197,14 @@
>         }
>  }
>
> +func TestOverlongMessagePKCS1v15(t *testing.T) {
> +       ciphertext := decodeBase64("**fjOVdirUzFoLlukv80dBllMLjXythI**
> f22feqPrNo0YoIjzyzyoMFiLjAc/**Y4krkeZ11XFThIrEvw\**nkRiZcCq5ng==")
> +       _, err := DecryptPKCS1v15(nil, rsaPrivateKey, ciphertext)
> +       if err == nil {
> +               t.Error("RSA decrypted a message that was too long.")
> +       }
> +}
> +
>  // In order to generate new test vectors you'll need the PEM form of this
> key:
>  // -----BEGIN RSA PRIVATE KEY-----
>  // MIIBOgIBAAJBALKZD0nEffqM1ACuak**0bijtqE2QrI/**KLADv7l3kK3ppMyCuLKoF0
> Index: src/pkg/crypto/subtle/**constant_time.go
> ==============================**==============================**=======
> --- a/src/pkg/crypto/subtle/**constant_time.go
> +++ b/src/pkg/crypto/subtle/**constant_time.go
> @@ -55,3 +55,11 @@
>         }
>         return
>  }
> +
> +// ConstantTimeLessOrEqual returns 1 if x <= y and 0 otherwise.
> +// Its behavior is undefined if x or y are negative or > 2**31 - 1.
> +func ConstantTimeLessOrEqual(x, y int) int {
> +       x32 := int32(x)
> +       y32 := int32(y)
> +       return int(((x32 - y32 - 1) >> 31) & 1)
> +}
> Index: src/pkg/crypto/subtle/**constant_time_test.go
> ==============================**==============================**=======
> --- a/src/pkg/crypto/subtle/**constant_time_test.go
> +++ b/src/pkg/crypto/subtle/**constant_time_test.go
> @@ -103,3 +103,23 @@
>                 t.Error(err)
>         }
>  }
> +
> +var lessOrEqualTests = []struct {
> +       x, y, result int
> +}{
> +       {0, 0, 1},
> +       {1, 0, 0},
> +       {0, 1, 1},
> +       {10, 20, 1},
> +       {20, 10, 0},
> +       {10, 10, 1},
> +}
> +
> +func TestConstantTimeLessOrEqual(t *testing.T) {
> +       for i, test := range lessOrEqualTests {
> +               result := ConstantTimeLessOrEqual(test.**x, test.y)
> +               if result != test.result {
> +                       t.Errorf("#%d: %d <= %d gave %d, expected %d", i,
> test.x, test.y, result, test.result)
> +               }
> +       }
> +}
>
>
> --
>
> ---You received this message because you are subscribed to the Google
> Groups "golang-dev" group.
> To unsubscribe from this group and stop receiving emails from it, send an
> email to
golang-dev+unsubscribe@**googlegroups.com<golang-dev%2Bunsubscribe@googlegrou...
> .
> For more options, visit
https://groups.google.com/**groups/opt_out<https://groups.google.com/groups/o...
> .
>
>
>

[agl1, 2013-05-14 23:03:37]

On Tue, May 14, 2013 at 6:09 PM, Brad Fitzpatrick <bradfitz@golang.org> wrote:
> Equal is spelled Eq elsewhere in crypto/subtle. Why inconsistent here?

Good point. Changed.


Cheers

AGL

[bradfitz, 2013-05-14 23:13:32]

LGTM



On Tue, May 14, 2013 at 2:43 PM, <agl@golang.org> wrote:

> Reviewers: golang-dev1,
>
> Message:
> Hello golang-dev@googlegroups.com (cc: golang-dev@googlegroups.com),
>
> I'd like you to review this change to
> https://code.google.com/p/go/
>
>
> Description:
> crypto/rsa: check for minimal PKCS#1 v1.5 padding.
>
> The PKCS#1 spec requires that the PS padding in an RSA message be at
> least 8 bytes long. We were not previously checking this. This isn't
> important in the most common situation (session key encryption), but
> the impact is unclear in other cases.
>
> This change enforces the specified minimum size.
>
> Please review this at
https://codereview.appspot.**com/9222045/<https://codereview.appspot.com/9222...
>
> Affected files:
>   M src/pkg/crypto/rsa/pkcs1v15.go
>   M src/pkg/crypto/rsa/pkcs1v15_**test.go
>   M src/pkg/crypto/subtle/**constant_time.go
>   M src/pkg/crypto/subtle/**constant_time_test.go
>
>
> Index: src/pkg/crypto/rsa/pkcs1v15.go
> ==============================**==============================**=======
> --- a/src/pkg/crypto/rsa/pkcs1v15.**go
> +++ b/src/pkg/crypto/rsa/pkcs1v15.**go
> @@ -124,7 +124,11 @@
>                 lookingForIndex = subtle.ConstantTimeSelect(**equals0, 0,
> lookingForIndex)
>         }
>
> -       valid = firstByteIsZero & secondByteIsTwo & (^lookingForIndex & 1)
> +       // The PS padding must be at least 8 bytes long, and it starts two
> +       // bytes into em.
> +       validPS := subtle.**ConstantTimeLessOrEqual(2+8, index)
> +
> +       valid = firstByteIsZero & secondByteIsTwo & (^lookingForIndex & 1)
> & validPS
>         msg = em[index+1:]
>         return
>  }
> Index: src/pkg/crypto/rsa/pkcs1v15_**test.go
> ==============================**==============================**=======
> --- a/src/pkg/crypto/rsa/pkcs1v15_**test.go
> +++ b/src/pkg/crypto/rsa/pkcs1v15_**test.go
> @@ -197,6 +197,14 @@
>         }
>  }
>
> +func TestOverlongMessagePKCS1v15(t *testing.T) {
> +       ciphertext := decodeBase64("**fjOVdirUzFoLlukv80dBllMLjXythI**
> f22feqPrNo0YoIjzyzyoMFiLjAc/**Y4krkeZ11XFThIrEvw\**nkRiZcCq5ng==")
> +       _, err := DecryptPKCS1v15(nil, rsaPrivateKey, ciphertext)
> +       if err == nil {
> +               t.Error("RSA decrypted a message that was too long.")
> +       }
> +}
> +
>  // In order to generate new test vectors you'll need the PEM form of this
> key:
>  // -----BEGIN RSA PRIVATE KEY-----
>  // MIIBOgIBAAJBALKZD0nEffqM1ACuak**0bijtqE2QrI/**KLADv7l3kK3ppMyCuLKoF0
> Index: src/pkg/crypto/subtle/**constant_time.go
> ==============================**==============================**=======
> --- a/src/pkg/crypto/subtle/**constant_time.go
> +++ b/src/pkg/crypto/subtle/**constant_time.go
> @@ -55,3 +55,11 @@
>         }
>         return
>  }
> +
> +// ConstantTimeLessOrEqual returns 1 if x <= y and 0 otherwise.
> +// Its behavior is undefined if x or y are negative or > 2**31 - 1.
> +func ConstantTimeLessOrEqual(x, y int) int {
> +       x32 := int32(x)
> +       y32 := int32(y)
> +       return int(((x32 - y32 - 1) >> 31) & 1)
> +}
> Index: src/pkg/crypto/subtle/**constant_time_test.go
> ==============================**==============================**=======
> --- a/src/pkg/crypto/subtle/**constant_time_test.go
> +++ b/src/pkg/crypto/subtle/**constant_time_test.go
> @@ -103,3 +103,23 @@
>                 t.Error(err)
>         }
>  }
> +
> +var lessOrEqualTests = []struct {
> +       x, y, result int
> +}{
> +       {0, 0, 1},
> +       {1, 0, 0},
> +       {0, 1, 1},
> +       {10, 20, 1},
> +       {20, 10, 0},
> +       {10, 10, 1},
> +}
> +
> +func TestConstantTimeLessOrEqual(t *testing.T) {
> +       for i, test := range lessOrEqualTests {
> +               result := ConstantTimeLessOrEqual(test.**x, test.y)
> +               if result != test.result {
> +                       t.Errorf("#%d: %d <= %d gave %d, expected %d", i,
> test.x, test.y, result, test.result)
> +               }
> +       }
> +}
>
>
> --
>
> ---You received this message because you are subscribed to the Google
> Groups "golang-dev" group.
> To unsubscribe from this group and stop receiving emails from it, send an
> email to
golang-dev+unsubscribe@**googlegroups.com<golang-dev%2Bunsubscribe@googlegrou...
> .
> For more options, visit
https://groups.google.com/**groups/opt_out<https://groups.google.com/groups/o...
> .
>
>
>

[agl1, 2013-05-15 14:27:46]

*** Submitted as https://code.google.com/p/go/source/detail?r=1c5529b1592a ***

crypto/rsa: check for minimal PKCS#1 v1.5 padding.

The PKCS#1 spec requires that the PS padding in an RSA message be at
least 8 bytes long. We were not previously checking this. This isn't
important in the most common situation (session key encryption), but
the impact is unclear in other cases.

This change enforces the specified minimum size.

R=golang-dev, bradfitz
CC=golang-dev
https://codereview.appspot.com/9222045