From 020d0bb0fbdad0bccd0bbbb398a3097e5d08c1f4 Mon Sep 17 00:00:00 2001 From: Mechiel Lukkien Date: Tue, 31 Jan 2023 00:22:26 +0100 Subject: [PATCH] add scram-sha-256 for smtp similar to imap. the code should be merged. this also reads the abort-line after authentication failure. --- imapserver/authenticate_test.go | 5 ++ imapserver/server.go | 7 +- smtp/codes.go | 2 +- smtpserver/parse.go | 17 ++++- smtpserver/server.go | 114 +++++++++++++++++++++++++++++--- 5 files changed, 128 insertions(+), 17 deletions(-) diff --git a/imapserver/authenticate_test.go b/imapserver/authenticate_test.go index 427cdc4..bf25657 100644 --- a/imapserver/authenticate_test.go +++ b/imapserver/authenticate_test.go @@ -90,6 +90,11 @@ func TestAuthenticateSCRAMSHA256(t *testing.T) { } else if err == nil || !errors.Is(err, serverFinalError) { t.Fatalf("server final, got err %#v, expected %#v", err, serverFinalError) } + if serverFinalError != nil { + tc.writelinef("*") + } else { + tc.writelinef("") + } _, result, err := tc.client.Response() tc.check(err, "read response") if string(result.Status) != strings.ToUpper(status) { diff --git a/imapserver/server.go b/imapserver/server.go index 8362ea5..cd379f0 100644 --- a/imapserver/server.go +++ b/imapserver/server.go @@ -1370,6 +1370,7 @@ func (c *conn) cmdAuthenticate(tag, cmd string, p *parser) { case "SCRAM-SHA-256": // todo: improve handling of errors during scram. e.g. invalid parameters. should we abort the imap command, or continue until the end and respond with a scram-level error? + // todo: use single implementation between ../imapserver/server.go and ../smtpserver/server.go authVariant = "scram-sha-256" @@ -1409,9 +1410,6 @@ func (c *conn) cmdAuthenticate(tag, cmd string, p *parser) { xcheckf(err, "read tx") }) s1, err := ss.ServerFirst(password.SCRAMSHA256.Iterations, password.SCRAMSHA256.Salt) - if err != nil { - xsyntaxErrorf("server first: %w", err) - } xcheckf(err, "scram first server step") c.writelinef("+ %s", base64.StdEncoding.EncodeToString([]byte(s1))) c2 := xreadContinuation() @@ -1420,6 +1418,7 @@ func (c *conn) cmdAuthenticate(tag, cmd string, p *parser) { c.writelinef("+ %s", base64.StdEncoding.EncodeToString([]byte(s3))) } if err != nil { + c.readline(false) // Should be "*" for cancellation. if errors.Is(err, scram.ErrInvalidProof) { authResult = "badcreds" xusercodeErrorf("AUTHENTICATIONFAILED", "bad credentials") @@ -2779,7 +2778,7 @@ func (c *conn) linkOrCopyFile(dst, src string) error { if df != nil { err = os.Remove(df.Name()) c.xsanity(err, "removing unfinished dst file") - df.Close() + err = df.Close() c.xsanity(err, "closing unfinished dst file") } }() diff --git a/smtp/codes.go b/smtp/codes.go index 81d7ab3..963c240 100644 --- a/smtp/codes.go +++ b/smtp/codes.go @@ -32,9 +32,9 @@ var ( C504ParamNotImpl = 504 C521HostNoMail = 521 // ../rfc/7504:179 C530SecurityRequired = 530 // ../rfc/3207:148 ../rfc/4954:623 - C538EncReqForAuth = 538 // ../rfc/4954:630 C534AuthMechWeak = 534 // ../rfc/4954:593 C535AuthBadCreds = 535 // ../rfc/4954:600 + C538EncReqForAuth = 538 // ../rfc/4954:630 C550MailboxUnavail = 550 C551UserNotLocal = 551 C552MailboxFull = 552 diff --git a/smtpserver/parse.go b/smtpserver/parse.go index 81f1ee2..3c7bb99 100644 --- a/smtpserver/parse.go +++ b/smtpserver/parse.go @@ -139,6 +139,21 @@ func (p *parser) takefn1(what string, fn func(c rune, i int) bool) string { return p.remainder() } +func (p *parser) takefn1case(what string, fn func(c rune, i int) bool) string { + if p.empty() { + p.xerrorf("need at least one char for %s", what) + } + for i, c := range p.orig[p.o:] { + if !fn(c, i) { + if i == 0 { + p.xerrorf("expected at least one char for %s", what) + } + return p.xtaken(i) + } + } + return p.remainder() +} + func (p *parser) takefn(fn func(c rune, i int) bool) string { for i, c := range p.upper[p.o:] { if !fn(c, i) { @@ -413,7 +428,7 @@ func (p *parser) xnumber(maxDigits int) int64 { // sasl mechanism, for AUTH command. // ../rfc/4422:436 func (p *parser) xsaslMech() string { - return p.takefn1("sasl-mech", func(c rune, i int) bool { + return p.takefn1case("sasl-mech", func(c rune, i int) bool { return i < 20 && (c >= 'A' && c <= 'Z' || c >= '0' && c <= '9' || c == '-' || c == '_') }) } diff --git a/smtpserver/server.go b/smtpserver/server.go index 5a9a0bd..1e3e2d0 100644 --- a/smtpserver/server.go +++ b/smtpserver/server.go @@ -21,6 +21,8 @@ import ( "github.com/prometheus/client_golang/prometheus" "github.com/prometheus/client_golang/prometheus/promauto" + "github.com/mjl-/bstore" + "github.com/mjl-/mox/config" "github.com/mjl-/mox/dkim" "github.com/mjl-/mox/dmarc" @@ -36,6 +38,7 @@ import ( "github.com/mjl-/mox/moxvar" "github.com/mjl-/mox/publicsuffix" "github.com/mjl-/mox/queue" + "github.com/mjl-/mox/scram" "github.com/mjl-/mox/smtp" "github.com/mjl-/mox/spf" "github.com/mjl-/mox/store" @@ -659,10 +662,9 @@ func (c *conn) cmdHello(p *parser, ehlo bool) { c.bwritelinef("250-STARTTLS") } if c.submission { - // todo future: implement SCRAM-SHA-256. // ../rfc/4954:123 if c.tls || !c.requireTLSForAuth { - c.bwritelinef("250-AUTH PLAIN") + c.bwritelinef("250-AUTH PLAIN SCRAM-SHA-256") } else { c.bwritelinef("250-AUTH ") } @@ -757,16 +759,8 @@ func (c *conn) cmdAuth(p *parser) { // ../rfc/4954:699 p.xspace() mech := p.xsaslMech() - switch mech { - case "PLAIN": - authVariant = "plain" - - // ../rfc/4954:343 - // ../rfc/4954:326 - if !c.tls && c.requireTLSForAuth { - xsmtpUserErrorf(smtp.C538EncReqForAuth, smtp.SePol7EncReqForAuth11, "authentication requires tls") - } + xreadInitial := func() []byte { var auth string if p.empty() { c.writelinef("%d ", smtp.C334ContinueAuth) // ../rfc/4954:205 @@ -793,6 +787,34 @@ func (c *conn) cmdAuth(p *parser) { // ../rfc/4954:235 xsmtpUserErrorf(smtp.C501BadParamSyntax, smtp.SeProto5Syntax2, "invalid base64: %s", err) } + return buf + } + + xreadContinuation := func() []byte { + line := c.readline() + if line == "*" { + authResult = "aborted" + xsmtpUserErrorf(smtp.C501BadParamSyntax, smtp.SeProto5Other0, "authentication aborted") + } + buf, err := base64.StdEncoding.DecodeString(line) + if err != nil { + // ../rfc/4954:235 + xsmtpUserErrorf(smtp.C501BadParamSyntax, smtp.SeProto5Syntax2, "invalid base64: %s", err) + } + return buf + } + + switch mech { + case "PLAIN": + authVariant = "plain" + + // ../rfc/4954:343 + // ../rfc/4954:326 + if !c.tls && c.requireTLSForAuth { + xsmtpUserErrorf(smtp.C538EncReqForAuth, smtp.SePol7EncReqForAuth11, "authentication requires tls") + } + + buf := xreadInitial() plain := bytes.Split(buf, []byte{0}) if len(plain) != 3 { xsmtpUserErrorf(smtp.C501BadParamSyntax, smtp.SeProto5BadParams4, "auth data should have 3 nul-separated tokens, got %d", len(plain)) @@ -819,6 +841,76 @@ func (c *conn) cmdAuth(p *parser) { // ../rfc/4954:276 c.writecodeline(smtp.C235AuthSuccess, smtp.SePol7Other0, "nice", nil) + case "SCRAM-SHA-256": + // todo: improve handling of errors during scram. e.g. invalid parameters. should we abort the imap command, or continue until the end and respond with a scram-level error? + // todo: use single implementation between ../imapserver/server.go and ../smtpserver/server.go + + authVariant = "scram-sha-256" + + c0 := xreadInitial() + ss, err := scram.NewServer(c0) + xcheckf(err, "starting scram") + c.log.Info("scram auth", mlog.Field("authentication", ss.Authentication)) + acc, _, err := store.OpenEmail(ss.Authentication) + if err != nil { + // todo: we could continue scram with a generated salt, deterministically generated + // from the username. that way we don't have to store anything but attackers cannot + // learn if an account exists. same for absent scram saltedpassword below. + xsmtpUserErrorf(smtp.C454TempAuthFail, smtp.SeSys3Other0, "scram not possible") + } + defer func() { + if acc != nil { + err := acc.Close() + if err != nil { + c.log.Errorx("closing account", err) + } + } + }() + if ss.Authorization != "" && ss.Authorization != ss.Authentication { + xsmtpUserErrorf(smtp.C535AuthBadCreds, smtp.SePol7AuthBadCreds8, "authentication with authorization for different user not supported") + } + var password store.Password + acc.WithRLock(func() { + err := acc.DB.Read(func(tx *bstore.Tx) error { + password, err = bstore.QueryTx[store.Password](tx).Get() + xsc := password.SCRAMSHA256 + if err == bstore.ErrAbsent || err == nil && (len(xsc.Salt) == 0 || xsc.Iterations == 0 || len(xsc.SaltedPassword) == 0) { + xsmtpUserErrorf(smtp.C454TempAuthFail, smtp.SeSys3Other0, "scram not possible") + } + xcheckf(err, "fetching credentials") + return err + }) + xcheckf(err, "read tx") + }) + s1, err := ss.ServerFirst(password.SCRAMSHA256.Iterations, password.SCRAMSHA256.Salt) + xcheckf(err, "scram first server step") + c.writelinef("%d %s", smtp.C334ContinueAuth, base64.StdEncoding.EncodeToString([]byte(s1))) // ../rfc/4954:187 + c2 := xreadContinuation() + s3, err := ss.Finish(c2, password.SCRAMSHA256.SaltedPassword) + if len(s3) > 0 { + c.writelinef("%d %s", smtp.C334ContinueAuth, base64.StdEncoding.EncodeToString([]byte(s3))) // ../rfc/4954:187 + } + if err != nil { + c.readline() // Should be "*" for cancellation. + if errors.Is(err, scram.ErrInvalidProof) { + authResult = "badcreds" + xsmtpUserErrorf(smtp.C535AuthBadCreds, smtp.SePol7AuthBadCreds8, "bad credentials") + } + xcheckf(err, "server final") + } + + // Client must still respond, but there is nothing to say. See ../rfc/9051:6221 + // The message should be empty. todo: should we require it is empty? + xreadContinuation() + + authResult = "ok" + c.authFailed = 0 + c.account = acc + acc = nil // Cancel cleanup. + c.username = ss.Authentication + // ../rfc/4954:276 + c.writecodeline(smtp.C235AuthSuccess, smtp.SePol7Other0, "nice", nil) + default: // todo future: implement scram-sha-256 ../rfc/7677 // todo future: possibly implement cram-md5, at least where we allow PLAIN. ../rfc/4954:348