net/smtp: most fields are not validated or sanitized

[rolandshoemaker]

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]

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]

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]

CC @neild maybe?

[rolandshoemaker]

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]

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]

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.

[DemiMarie]

Personally I would validate according to the RFC.

[mpx]

For reference, RFC 5321 Section 4.1.2 describes the formats.

Perhaps the full Path is not required - no need to support source routing? This could simplify validation a bit to just Mailbox - but it would still need to support quoted strings.