code review 6348074: encoding/asn1: promote untyped strings to UTF8 as needed.

encoding/asn1: promote untyped strings to UTF8 as needed.

Previously, strings that didn't have an explicit ASN.1 string type
were taken to be ASN.1 PrintableStrings. This resulted in an error if
a unrepresentable charactor was included.

For compatibility reasons, I'm too afraid to switch the default string
type to UTF8String, but this patch causes untyped strings to become
UTF8Strings if they contain a charactor that's not valid in a
PrintableString.

Fixes issue 3791.

[agl1, 2012-07-09 15:20: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, 2012-07-09 17:04:11]

With the caveat that I don't know ASN.1 at all, what about keeping it
printable with the logic as you have it now (all runes <= 255 and
printable) but only setting the string type to UTF-8 if
utf8.ValidString(string)?  I'm thinking of binary data in an ASN.1 string
being false-positive flagged as UTF-8.

On Mon, Jul 9, 2012 at 8:20 AM, <agl@golang.org> wrote:

> Reviewers: golang-dev_googlegroups.com,
>
> 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:
> encoding/asn1: promote untyped strings to UTF8 as needed.
>
> Previously, strings that didn't have an explicit ASN.1 string type
> were taken to be ASN.1 PrintableStrings. This resulted in an error if
> a unrepresentable charactor was included.
>
> For compatibility reasons, I'm too afraid to switch the default string
> type to UTF8String, but this patch causes untyped strings to become
> UTF8Strings if they contain a charactor that's not valid in a
> PrintableString.
>
> Fixes issue 3791.
>
> Please review this at
http://codereview.appspot.com/**6348074/<http://codereview.appspot.com/6348074/>
>
> Affected files:
>   M src/pkg/crypto/x509/x509_test.**go
>   M src/pkg/encoding/asn1/common.**go
>   M src/pkg/encoding/asn1/marshal.**go
>   M src/pkg/encoding/asn1/marshal_**test.go
>
>
> Index: src/pkg/crypto/x509/x509_test.**go
> ==============================**==============================**=======
> --- a/src/pkg/crypto/x509/x509_**test.go
> +++ b/src/pkg/crypto/x509/x509_**test.go
> @@ -251,7 +251,7 @@
>                 SerialNumber: big.NewInt(1),
>                 Subject: pkix.Name{
>                         CommonName:   commonName,
> -                       Organization: []string{"Acme Co"},
> +                       Organization: []string{"Σ Acme Co"},
>                 },
>                 NotBefore: time.Unix(1000, 0),
>                 NotAfter:  time.Unix(100000, 0),
> Index: src/pkg/encoding/asn1/common.**go
> ==============================**==============================**=======
> --- a/src/pkg/encoding/asn1/**common.go
> +++ b/src/pkg/encoding/asn1/**common.go
> @@ -98,6 +98,8 @@
>                         ret.stringType = tagIA5String
>                 case part == "printable":
>                         ret.stringType = tagPrintableString
> +               case part == "utf8":
> +                       ret.stringType = tagUTF8String
>                 case strings.HasPrefix(part, "default:"):
>                         i, err := strconv.ParseInt(part[8:], 10, 64)
>                         if err == nil {
> Index: src/pkg/encoding/asn1/marshal.**go
> ==============================**==============================**=======
> --- a/src/pkg/encoding/asn1/**marshal.go
> +++ b/src/pkg/encoding/asn1/**marshal.go
> @@ -280,6 +280,11 @@
>         return
>  }
>
> +func marshalUTF8String(out *forkableWriter, s string) (err error) {
> +       _, err = out.Write([]byte(s))
> +       return
> +}
> +
>  func marshalTwoDigits(out *forkableWriter, v int) (err error) {
>         err = out.WriteByte(byte('0' + (v/10)%10))
>         if err != nil {
> @@ -446,10 +451,13 @@
>                 }
>                 return
>         case reflect.String:
> -               if params.stringType == tagIA5String {
> +               switch params.stringType {
> +               case tagIA5String:
>                         return marshalIA5String(out, v.String())
> -               } else {
> +               case tagPrintableString:
>                         return marshalPrintableString(out, v.String())
> +               default:
> +                       return marshalUTF8String(out, v.String())
>                 }
>                 return
>         }
> @@ -492,11 +500,24 @@
>         }
>         class := classUniversal
>
> -       if params.stringType != 0 {
> -               if tag != tagPrintableString {
> -                       return StructuralError{"Explicit string type given
> to non-string member"}
> +       if params.stringType != 0 && tag != tagPrintableString {
> +               return StructuralError{"Explicit string type given to
> non-string member"}
> +       }
> +
> +       if tag == tagPrintableString {
> +               if params.stringType == 0 {
> +                       // This is a string without an explicit string
> type. We'll use
> +                       // a PrintableString if the charactor set in the
> string is
> +                       // sufficiently limited, otherwise we'll use a
> UTF8String.
> +                       for _, r := range v.String() {
> +                               if r > 255 || !isPrintable(byte(r)) {
> +                                       tag = tagUTF8String
> +                                       break
> +                               }
> +                       }
> +               } else {
> +                       tag = params.stringType
>                 }
> -               tag = params.stringType
>         }
>
>         if params.set {
> Index: src/pkg/encoding/asn1/marshal_**test.go
> ==============================**==============================**=======
> --- a/src/pkg/encoding/asn1/**marshal_test.go
> +++ b/src/pkg/encoding/asn1/**marshal_test.go
> @@ -122,6 +122,7 @@
>         {testSET([]int{10}), "310302010a"},
>         {omitEmptyTest{[]string{}}, "3000"},
>         {omitEmptyTest{[]string{"1"}}, "30053003130131"},
> +       {"Σ", "0c02cea3"},
>  }
>
>  func TestMarshal(t *testing.T) {
>
>
>

[agl1, 2012-07-09 17:11:35]

On Mon, Jul 9, 2012 at 1:04 PM, Brad Fitzpatrick <bradfitz@golang.org> wrote:
> With the caveat that I don't know ASN.1 at all, what about keeping it
> printable with the logic as you have it now (all runes <= 255 and printable)
> but only setting the string type to UTF-8 if utf8.ValidString(string)?  I'm
> thinking of binary data in an ASN.1 string being false-positive flagged as
> UTF-8.

That's a fair point. I've updated the logic to check utf8.ValidString
before encoding as a UTF-8 string. If it's invalid then we return an
error.


Cheers

AGL

[bradfitz, 2012-07-09 17:19:08]

LGTM

On Mon, Jul 9, 2012 at 10:11 AM, Adam Langley <agl@golang.org> wrote:

> On Mon, Jul 9, 2012 at 1:04 PM, Brad Fitzpatrick <bradfitz@golang.org>
> wrote:
> > With the caveat that I don't know ASN.1 at all, what about keeping it
> > printable with the logic as you have it now (all runes <= 255 and
> printable)
> > but only setting the string type to UTF-8 if utf8.ValidString(string)?
>  I'm
> > thinking of binary data in an ASN.1 string being false-positive flagged
> as
> > UTF-8.
>
> That's a fair point. I've updated the logic to check utf8.ValidString
> before encoding as a UTF-8 string. If it's invalid then we return an
> error.
>
>
> Cheers
>
> AGL
>

[r, 2012-07-09 22:12:56]

http://codereview.appspot.com/6348074/diff/10001/src/pkg/encoding/asn1/marsha...
File src/pkg/encoding/asn1/marshal.go (right):

http://codereview.appspot.com/6348074/diff/10001/src/pkg/encoding/asn1/marsha...
src/pkg/encoding/asn1/marshal.go:515: if r > 255 || !isPrintable(byte(r)) {
surely 127 or (better) >= 0x80
the combination of isPrintable and this check is odd. if the byte is, say, 0x00,
why does it change from ASCII to UTF8?

[agl1, 2012-07-09 22:19:08]

http://codereview.appspot.com/6348074/diff/10001/src/pkg/encoding/asn1/marsha...
File src/pkg/encoding/asn1/marshal.go (right):

http://codereview.appspot.com/6348074/diff/10001/src/pkg/encoding/asn1/marsha...
src/pkg/encoding/asn1/marshal.go:515: if r > 255 || !isPrintable(byte(r)) {
On 2012/07/09 22:12:56, r wrote:
> surely 127 or (better) >= 0x80
> the combination of isPrintable and this check is odd. if the byte is, say,
0x00,
> why does it change from ASCII to UTF8?

a PRINTABLESTRING isn't ASCII, it's a much more limited subset:

// isPrintable returns true iff the given b is in the ASN.1 PrintableString set.
func isPrintable(b byte) bool {
»·······return 'a' <= b && b <= 'z' ||
»·······»·······'A' <= b && b <= 'Z' ||
»·······»·······'0' <= b && b <= '9' ||
»·······»·······'\'' <= b && b <= ')' ||
»·······»·······'+' <= b && b <= '/' ||
»·······»·······b == ' ' ||
»·······»·······b == ':' ||
»·······»·······b == '=' ||
»·······»·······b == '?' ||
»·······»·······// This is technically not allowed in a PrintableString.
»·······»·······// However, x509 certificates with wildcard strings don't
»·······»·······// always use the correct string type so we permit it.
»·······»·······b == '*'
}

So the check for > 255 might be better written as r == rune(byte(r)) to make it
clear that what it's checking is that we aren't aliasing two runes with the byte
conversion.

[r, 2012-07-09 22:22:59]

http://codereview.appspot.com/6348074/diff/10001/src/pkg/encoding/asn1/marsha...
File src/pkg/encoding/asn1/marshal.go (right):

http://codereview.appspot.com/6348074/diff/10001/src/pkg/encoding/asn1/marsha...
src/pkg/encoding/asn1/marshal.go:515: if r > 255 || !isPrintable(byte(r)) {
if that's what you're after, the question is r >= utf8.RuneSelf, which is 0x80
not 0xFF.

[r, 2012-07-09 22:24:55]

checking my understanding: if the byte is not PRINTABLE, it must be part of a
valid UTF-8 string and will be identified as such in the marshaled data.

[agl1, 2012-07-09 22:29:02]

On 2012/07/09 22:24:55, r wrote:
> checking my understanding: if the byte is not PRINTABLE, it must be part of a
> valid UTF-8 string and will be identified as such in the marshaled data.

Right. At the moment we serialise ASN.1 structures often without setting an
explicit string type in the struct. So if someone puts a non-PRINTABLE string in
there, Marshal fails because it defaults to PRINTABLESTRING.

We could also fix this by marking all the strings as UTF8 explicitly, but that
would cause them to always be ASN.1 UTF8STRINGs. That is technically valid, but
I expect it to be incompatible with something out there.

So the intent here is that, if the string in the struct isn't explicitly typed,
then we'll use PRINTABLESTRING if possible, and UTF8STRING otherwise. If the
string isn't valid UTF8 either, then we'll throw an error.

[r2, 2012-07-10 19:51:05]

this CL isn't viewable by me: chunk mismatch

-rob

[agl1, 2012-07-10 19:54:53]

uploaded again and codereview seems to be happy now.

[r, 2012-07-10 20:07:46]

LGTM after fixing buglet

http://codereview.appspot.com/6348074/diff/2002/src/pkg/encoding/asn1/marshal.go
File src/pkg/encoding/asn1/marshal.go (right):

http://codereview.appspot.com/6348074/diff/2002/src/pkg/encoding/asn1/marshal...
src/pkg/encoding/asn1/marshal.go:515: if r > utf8.RuneSelf ||
!isPrintable(byte(r)) {
s/>/>=/

[agl1, 2012-07-10 22:23:47]

*** Submitted as http://code.google.com/p/go/source/detail?r=63d9b9b595c5 ***

encoding/asn1: promote untyped strings to UTF8 as needed.

Previously, strings that didn't have an explicit ASN.1 string type
were taken to be ASN.1 PrintableStrings. This resulted in an error if
a unrepresentable charactor was included.

For compatibility reasons, I'm too afraid to switch the default string
type to UTF8String, but this patch causes untyped strings to become
UTF8Strings if they contain a charactor that's not valid in a
PrintableString.

Fixes issue 3791.

R=golang-dev, bradfitz, r, r
CC=golang-dev
http://codereview.appspot.com/6348074

http://codereview.appspot.com/6348074/diff/2002/src/pkg/encoding/asn1/marshal.go
File src/pkg/encoding/asn1/marshal.go (right):

http://codereview.appspot.com/6348074/diff/2002/src/pkg/encoding/asn1/marshal...
src/pkg/encoding/asn1/marshal.go:515: if r > utf8.RuneSelf ||
!isPrintable(byte(r)) {
On 2012/07/10 20:07:46, r wrote:
> s/>/>=/

Done.