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

net/smtp: most fields are not validated or sanitized #66146

Open
rolandshoemaker opened this issue Mar 6, 2024 · 6 comments
Open

net/smtp: most fields are not validated or sanitized #66146

rolandshoemaker opened this issue Mar 6, 2024 · 6 comments
Labels
NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. Security
Milestone

Comments

@rolandshoemaker
Copy link
Member

The net/smtp package is somewhat dangerous, in that it doesn't really do any validation of data passed to it, and generally will just directly pass that through to the SMTP server without any sanitization. For certain methods, like Client.Mail, or functions like SendMail this can cause issues if the to/from addresses are malformed, or contain reserved characters (like angle brackets), or if headers are malformed, etc. This is may have significant impact if the caller of this API is accepting untrusted user input and using it to populate these arguments without doing their own validation.

We could add validation to various parts of the package, but that would be a rather significant change in an otherwise frozen package. At the least we should document the (as currently defined) security properties of this package (i.e., there are none) and make it clear that this package is not hardened against adversarial inputs, and users should be extremely careful about what they pass into these functions/methods.

Thanks to RyotaK (@Ry0taK) for reporting this issue.

@mpx
Copy link
Contributor

mpx commented Mar 7, 2024

There are ~7500 imports shown on pkg.go.dev (not de-duped) which is ~35x the next most popular "smtp" package. It's also used by popular projects (eg, github.com/prometheus/alertmanager). I'm sure there are many more uses in private codebases.

AIUI, "frozen" means that people shouldn't expect additional feature development. The API doesn't provide a huge number of features, but it's often plenty for most use cases. I would have expected that security issues would still be addressed in frozen packages (and especially if they are popular).

@bjorndm
Copy link

bjorndm commented Mar 7, 2024

If net/smnp is not secure, it should be made secure since a lot of developers, myself included, use it. The go compatibility promise may be broken for security reasons, so I would be perfectly fine if this package was updated for security even if it is not completely compatible. Maybe even consider unfreezing it seeing how widely it is used.

@mknyszek
Copy link
Contributor

mknyszek commented Mar 7, 2024

CC @neild maybe?

@mknyszek mknyszek added this to the Backlog milestone Mar 7, 2024
@mknyszek mknyszek added the NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. label Mar 7, 2024
@rolandshoemaker
Copy link
Member Author

I think the question here is where we draw the security boundary. RFC 5321 (and associated RFCs) has ABNFs for a lot of things, should we be enforcing all of them for all of the APIs?

Checking addresses passed to Client.Mail and Client.Rcpt etc seems reasonable, but SendMail documents that the msg argument should be a RFC 822 formatted email, should we be enforcing that it strictly confirms to that style? SendMail is also a very low level API, it expects the caller to have assembled the email, so there is a question as to what degree it should be hardened against adversarial input, vs assuming that the caller has done some level of validation themselves.

SMTP is one of those things where specification and reality don't perfectly intersect, so I suspect if we were to fully enforce the letter of specifications we'd end up breaking some real world use cases that tend to Just Work currently.

It may be reasonable to draw the boundary at arguments passed to Client methods (which gives us a relatively restricted set of ABNFs to enforce), and say that anything you write to the io.WriteCloser returned by Client.Data, or the content of the msg argument passed to SendMail should be RFC 822 compliant, but not strictly enforce that.

@rolandshoemaker
Copy link
Member Author

The model for the above proposed changes would be: for anything we construct (commands, headers, etc) from user supplied inputs we will check validity to make sure we've generated well formed output. For anything user constructed (message bodies, etc) we will assume the user has done that validation themselves.

@neild
Copy link
Contributor

neild commented Mar 11, 2024

So, as a concrete example:

client.Mail(`">"@go.dev`)
// MAIL FROM: <">"@go.dev>

I'm pretty sure that's a valid MAIL FROM line. Should we be doing validation that allows it, but doesn't allow:

client.Mail("address@go.dev> EVIL=yes FOO=")
// MAIL FROM: <address@go.dev> EVIL=yes FOO=>

In this case, we've got a MAIL FROM that has an unexpected pair of mail-parameters. This requires that the terminal > can get slid into the MAIL FROM somehow without the recipient rejecting it, and I can't think of anything excitingly evil you can do by injecting parameters in this way, but let's grant that we probably shouldn't allow this.

We could just reject > in the address, even though it's technically allowed. Or we could validate everything according to the RFC 5321 ABNF, but I'm not convinced that's sufficient defense; the first example above is technically valid but possibly surprising.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. Security
Projects
None yet
Development

No branches or pull requests

5 participants