Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Maybe I am missing something #77

Closed
awalterschulze opened this issue Sep 16, 2015 · 9 comments
Closed

Maybe I am missing something #77

awalterschulze opened this issue Sep 16, 2015 · 9 comments

Comments

@awalterschulze
Copy link

Given this message

message Nil {

}

Yes with no fields, so everything is XXX_unrecognized

Hopefully the comments help.

Given this input

[]byte{0x9, 0xbd, 0x67, 0x7f, 0x8c, 0x2a, 0x43, 0xee, 0x3f, 0x15, 0xf8, 0x29, 0x44, 0xbf, 0x1a, 0xa, 0x30, 0xd7, 0x9a, 0xec, 0xf1, 0xd, 0x38, 0x89, 0xaf, 0x8c, 0x1d, 0x4d, 0xfb, 0x62, 0xa8, 0xf9, 0x55, 0xa, 0x6d, 0x55, 0x3b, 0x59, 0xa2, 0xce, 0x5f, /*check here*/ 0x99, 0x0, 0x0, 0x0, 0x0, 0x61, /*the rest is normal*/  0xba, 0xa5, 0x72, 0x14, 0x38, 0xe8, 0xf2, 0x1d, 0x68, 0x0, 0x72, 0x57, 0x72, 0x31, 0x51, 0x56, 0x4a, 0x78, 0x64, 0x79, 0x72, 0x6a, 0x64, 0x4e, 0x66, 0x31, 0x72, 0x65, 0x4c, 0x47, 0x30, 0x39, 0x43, 0x70, 0x4e, 0x35, 0x5a, 0x78, 0x4d, 0x65, 0x71, 0x70, 0x68, 0x31, 0x57, 0x6d, 0x69, 0x50, 0x66, 0x77, 0x38, 0x79, 0x61, 0x5a, 0x61, 0x4c, 0x51, 0x72, 0x47, 0x4f, 0x73, 0x52, 0x4b, 0x32, 0x77, 0x63, 0x6d, 0x50, 0x52, 0x37, 0x75, 0x6c, 0x48, 0x74, 0x72, 0x59, 0x69, 0x38, 0x75, 0x51, 0x53, 0x4e, 0x6d, 0x54, 0x31, 0x47, 0x57, 0x42, 0x43, 0x4d, 0x78, 0x35, 0x64, 0x45, 0x4c, 0x4e, 0x49, 0x6d, 0x69, 0x7a, 0x45, 0x64, 0x20, 0x20, 0x4, 0x6a, 0x4a, 0xe2, 0xe7, 0x41, 0xf1, 0x7c, 0x88, 0x73, 0xfa, 0xcb, 0xed, 0x7e, 0xd, 0x66, 0xc2, 0x31, 0xc8, 0xbc, 0x93, 0xaf, 0xda, 0xae, 0xfd, 0xd2, 0x47, 0x4f, 0x63, 0x86, 0x12, 0x9, 0x20, 0x31, 0x97, 0x78, 0xa9, 0xb4, 0x0, 0xa6, 0x44, 0x93, 0xa9, 0x38, 0xc, 0x69, 0xaf, 0xbb, 0xdd, 0x68, 0x68, 0x58, 0x3c, 0xe, 0x51, 0x39, 0xe9, 0xf6, 0x5e, 0xac, 0x38, 0x15, 0xd3, 0x31, 0x30, 0x30}

the message becomes

&Nil{XXX_unrecognized:
[]byte{0x9, 0xbd, 0x67, 0x7f, 0x8c, 0x2a, 0x43, 0xee, 0x3f, 0x15, 0xf8, 0x29, 0x44, 0xbf, 0x1a, 0xa, 0x30, 0xd7, 0x9a, 0xec, 0xf1, 0xd, 0x38, 0x89, 0xaf, 0x8c, 0x1d, 0x4d, 0xfb, 0x62, 0xa8, 0xf9, 0x55, 0xa, 0x6d, 0x55, 0x3b, 0x59, 0xa2, 0xce, 0x5f, /*check here*/ 0x19, 0x0, 0x0, 0x0, 0x61, /*the rest is normal*/ 0xba, 0xa5, 0x72, 0x14, 0x38, 0xe8, 0xf2, 0x1d, 0x68, 0x0, 0x72, 0x57, 0x72, 0x31, 0x51, 0x56, 0x4a, 0x78, 0x64, 0x79, 0x72, 0x6a, 0x64, 0x4e, 0x66, 0x31, 0x72, 0x65, 0x4c, 0x47, 0x30, 0x39, 0x43, 0x70, 0x4e, 0x35, 0x5a, 0x78, 0x4d, 0x65, 0x71, 0x70, 0x68, 0x31, 0x57, 0x6d, 0x69, 0x50, 0x66, 0x77, 0x38, 0x79, 0x61, 0x5a, 0x61, 0x4c, 0x51, 0x72, 0x47, 0x4f, 0x73, 0x52, 0x4b, 0x32, 0x77, 0x63, 0x6d, 0x50, 0x52, 0x37, 0x75, 0x6c, 0x48, 0x74, 0x72, 0x59, 0x69, 0x38, 0x75, 0x51, 0x53, 0x4e, 0x6d, 0x54, 0x31, 0x47, 0x57, 0x42, 0x43, 0x4d, 0x78, 0x35, 0x64, 0x45, 0x4c, 0x4e, 0x49, 0x6d, 0x69, 0x7a, 0x45, 0x64, 0x20, 0x20, 0x4, 0x6a, 0x4a, 0xe2, 0xe7, 0x41, 0xf1, 0x7c, 0x88, 0x73, 0xfa, 0xcb, 0xed, 0x7e, 0xd, 0x66, 0xc2, 0x31, 0xc8, 0xbc, 0x93, 0xaf, 0xda, 0xae, 0xfd, 0xd2, 0x47, 0x4f, 0x63, 0x86, 0x12, 0x9, 0x20, 0x31, 0x97, 0x78, 0xa9, 0xb4, 0x0, 0xa6, 0x44, 0x93, 0xa9, 0x38, 0xc, 0x69, 0xaf, 0xbb, 0xdd, 0x68, 0x68, 0x58, 0x3c, 0xe, 0x51, 0x39, 0xe9, 0xf6, 0x5e, 0xac, 0x38, 0x15, 0xd3, 0x31, 0x30, 0x30}

Is this working as expected?

@awalterschulze
Copy link
Author

This was found given this version of the fuzztests code
https://github.com/gogo/fuzztests/tree/07c9f503e600292403cfcb4d33b5542e20ad450c

@dsymonds
Copy link
Contributor

Is there a difference between those two byte lists? They look at a glance to be the same, which is what I'd expect.

@awalterschulze
Copy link
Author

They should be same, but they are not.
Let me try and make them more glance-able

[]byte{0x9, 0xbd, 0x67, 0x7f, 0x8c, 0x2a, 0x43, 0xee, 0x3f, 0x15, 0xf8, 0x29, 0x44, 0xbf, 0x1a, 0xa, 0x30, 0xd7, 0x9a, 0xec, 0xf1, 0xd, 0x38, 0x89, 0xaf, 0x8c, 0x1d, 0x4d, 0xfb, 0x62, 0xa8, 0xf9, 0x55, 0xa, 0x6d, 0x55, 0x3b, 0x59, 0xa2, 0xce, 0x5f, 
/*check here*/ 0x99, 0x0, 0x0, 0x0, 0x0, 0x61, 
/*the rest is normal*/  0xba, 0xa5, 0x72, 0x14, 0x38, 0xe8, 0xf2, 0x1d, 0x68, 0x0, 0x72, 0x57, 0x72, 0x31, 0x51, 0x56, 0x4a, 0x78, 0x64, 0x79, 0x72, 0x6a, 0x64, 0x4e, 0x66, 0x31, 0x72, 0x65, 0x4c, 0x47, 0x30, 0x39, 0x43, 0x70, 0x4e, 0x35, 0x5a, 0x78, 0x4d, 0x65, 0x71, 0x70, 0x68, 0x31, 0x57, 0x6d, 0x69, 0x50, 0x66, 0x77, 0x38, 0x79, 0x61, 0x5a, 0x61, 0x4c, 0x51, 0x72, 0x47, 0x4f, 0x73, 0x52, 0x4b, 0x32, 0x77, 0x63, 0x6d, 0x50, 0x52, 0x37, 0x75, 0x6c, 0x48, 0x74, 0x72, 0x59, 0x69, 0x38, 0x75, 0x51, 0x53, 0x4e, 0x6d, 0x54, 0x31, 0x47, 0x57, 0x42, 0x43, 0x4d, 0x78, 0x35, 0x64, 0x45, 0x4c, 0x4e, 0x49, 0x6d, 0x69, 0x7a, 0x45, 0x64, 0x20, 0x20, 0x4, 0x6a, 0x4a, 0xe2, 0xe7, 0x41, 0xf1, 0x7c, 0x88, 0x73, 0xfa, 0xcb, 0xed, 0x7e, 0xd, 0x66, 0xc2, 0x31, 0xc8, 0xbc, 0x93, 0xaf, 0xda, 0xae, 0xfd, 0xd2, 0x47, 0x4f, 0x63, 0x86, 0x12, 0x9, 0x20, 0x31, 0x97, 0x78, 0xa9, 0xb4, 0x0, 0xa6, 0x44, 0x93, 0xa9, 0x38, 0xc, 0x69, 0xaf, 0xbb, 0xdd, 0x68, 0x68, 0x58, 0x3c, 0xe, 0x51, 0x39, 0xe9, 0xf6, 0x5e, 0xac, 0x38, 0x15, 0xd3, 0x31, 0x30, 0x30}
[]byte{0x9, 0xbd, 0x67, 0x7f, 0x8c, 0x2a, 0x43, 0xee, 0x3f, 0x15, 0xf8, 0x29, 0x44, 0xbf, 0x1a, 0xa, 0x30, 0xd7, 0x9a, 0xec, 0xf1, 0xd, 0x38, 0x89, 0xaf, 0x8c, 0x1d, 0x4d, 0xfb, 0x62, 0xa8, 0xf9, 0x55, 0xa, 0x6d, 0x55, 0x3b, 0x59, 0xa2, 0xce, 0x5f, 
/*check here*/ 0x19, 0x0, 0x0, 0x0, 0x61, 
/*the rest is normal*/ 0xba, 0xa5, 0x72, 0x14, 0x38, 0xe8, 0xf2, 0x1d, 0x68, 0x0, 0x72, 0x57, 0x72, 0x31, 0x51, 0x56, 0x4a, 0x78, 0x64, 0x79, 0x72, 0x6a, 0x64, 0x4e, 0x66, 0x31, 0x72, 0x65, 0x4c, 0x47, 0x30, 0x39, 0x43, 0x70, 0x4e, 0x35, 0x5a, 0x78, 0x4d, 0x65, 0x71, 0x70, 0x68, 0x31, 0x57, 0x6d, 0x69, 0x50, 0x66, 0x77, 0x38, 0x79, 0x61, 0x5a, 0x61, 0x4c, 0x51, 0x72, 0x47, 0x4f, 0x73, 0x52, 0x4b, 0x32, 0x77, 0x63, 0x6d, 0x50, 0x52, 0x37, 0x75, 0x6c, 0x48, 0x74, 0x72, 0x59, 0x69, 0x38, 0x75, 0x51, 0x53, 0x4e, 0x6d, 0x54, 0x31, 0x47, 0x57, 0x42, 0x43, 0x4d, 0x78, 0x35, 0x64, 0x45, 0x4c, 0x4e, 0x49, 0x6d, 0x69, 0x7a, 0x45, 0x64, 0x20, 0x20, 0x4, 0x6a, 0x4a, 0xe2, 0xe7, 0x41, 0xf1, 0x7c, 0x88, 0x73, 0xfa, 0xcb, 0xed, 0x7e, 0xd, 0x66, 0xc2, 0x31, 0xc8, 0xbc, 0x93, 0xaf, 0xda, 0xae, 0xfd, 0xd2, 0x47, 0x4f, 0x63, 0x86, 0x12, 0x9, 0x20, 0x31, 0x97, 0x78, 0xa9, 0xb4, 0x0, 0xa6, 0x44, 0x93, 0xa9, 0x38, 0xc, 0x69, 0xaf, 0xbb, 0xdd, 0x68, 0x68, 0x58, 0x3c, 0xe, 0x51, 0x39, 0xe9, 0xf6, 0x5e, 0xac, 0x38, 0x15, 0xd3, 0x31, 0x30, 0x30}

@dsymonds
Copy link
Contributor

Thanks, that's easier to see.

So the input difference starts with 0x99, 0x0. In the wire format, that's a varint of value 25, but encoded inefficiently (it should just be 0x19), which is what is in the output. So the parser is picking up the bytes, decoding them, and writing down equivalent (but different, and simpler) bytes.

I don't think we guarantee that we preserve the bytes exactly in a situation like that, so I'm going to close this as a non-bug.

@awalterschulze
Copy link
Author

Ok excellent, I thought I was missing something.
I wonder how I can defend against that type of fuzzing, hmmm.

@dsymonds
Copy link
Contributor

I'm not sure what there is to "defend" against. There are multiple ways of encoding the same thing in protocol buffers; this is just one of them.

@awalterschulze
Copy link
Author

I know, but if I can sniff out those cases I can test the other cases where it should be the same better.
I found quite a few bugs already where gogoprotobuf and maybe even golang/protobuf throws away some unrecognized bytes that contains some errors without returning an error.
I also found and fixed a case where gogoprotobuf misses an unrecognized byte, probably because of the multiple encodings of a varint.
I haven't reported them yet, since I am still investigating and I need to fix these two errors first to get rid of the noise.

  • I check that no merging was used
  • I check that packed was not used
  • I also check that an XXX_unrecognized field is present

The next thing is to check that all varints are efficiently encoded.
Do you know of any other?

@dsymonds
Copy link
Contributor

There's too many equivalent-but-different encodings. I think you're tilting at windmills. For instance, tag order in a message is arbitrary. Map entries can be random (and key/value can be in any order inside that). Extensions obscure stuff too. There's probably others.

@awalterschulze
Copy link
Author

I might be, but I am only checking the length so all the order cases are not checked, since that would be basically impossible.
I am not testing extensions or enums since the libraries can't handle multiple versions being registered with the same name.
Maybe you can think of others?

@golang golang locked as resolved and limited conversation to collaborators Jun 26, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants