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.

Sign up for GitHub

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

Jump to bottom

passing err.Error() to the format string of grpc.Errorf allows format string injection #396

Closed
petermattis opened this issue Feb 9, 2021 · 2 comments · Fixed by #397
Closed

passing err.Error() to the format string of grpc.Errorf allows format string injection #396

petermattis opened this issue Feb 9, 2021 · 2 comments · Fixed by #397

Comments

@petermattis
Copy link
Contributor

go-grpc-middleware/validator/validator.go

Line 25 in fab13c2

return nil, status.Errorf(codes.InvalidArgument, err.Error())

I think the call to grpc.Errorf should look something like grpc.Errorf(codes.InvalidArgument, "%s", err.Error()). Alternately, you could call status.Error instead since grpc.Errorf is a wrapper around status.Errorf.
@petermattis
Copy link
Contributor Author

The title isn't quite accurate. I don't think this causes any problems in practice beyond a confusing error string, but it deserves to be fixed.

@johanbrandhorst
Copy link
Collaborator

Good spot, that is indeed wrong, would you like to contribute a fix?

petermattis added a commit to petermattis/go-grpc-middleware that referenced this issue Feb 11, 2021
@petermattis
use status.Error instead of status.Errorf
5fdd494 
Use `status.Error` instead of `status.Errorf` when the format string is
non-constant and not actually a format string. In the case of the
validator middleware, the error being supplied as a format string could
potentially contain data supplied by an attacker allowing for format
string injection. This doesn't appear to be an actual problem due to
`fmt` being safe in this regards, but it certainly isn't good practice
to provide a format string that an attacker can control.

Fixes grpc-ecosystem#396
@petermattis petermattis mentioned this issue Feb 11, 2021
Merged
johanbrandhorst pushed a commit that referenced this issue Feb 11, 2021
@petermattis
use status.Error instead of status.Errorf (#397)
912313c 
Use `status.Error` instead of `status.Errorf` when the format string is
non-constant and not actually a format string. In the case of the
validator middleware, the error being supplied as a format string could
potentially contain data supplied by an attacker allowing for format
string injection. This doesn't appear to be an actual problem due to
`fmt` being safe in this regards, but it certainly isn't good practice
to provide a format string that an attacker can control.

Fixes #396
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

use status.Error instead of status.Errorf petermattis/go-grpc-middleware
2 participants
@petermattis @johanbrandhorst