1
\$\begingroup\$

I'm writing login system in Go(Golang) using cookies.I think it's isn't safe enough. Can you provide some suggestions on how to improve the security.
Main file:

package main

import (
    "fmt"
    "golang.org/x/crypto/bcrypt"
    "html/template"
    "math/rand"
    "net/http"
    "strings"
    "time"
)

var (
    runes  = []rune("abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ01234567890")
    t *template.Template
)

func init()  {
    rand.Seed(time.Now().Unix())
    t, _ = template.ParseFiles("main.html","signup.html","signin.html")
}

type User struct {
    Login, Email string
}



func genToken() string {
    s := make([]rune, 15)
    for i := range s {
        s[i] = runes[rand.Intn(len(runes))]
    }
    return string(s)
}


func setCookie(w http.ResponseWriter, name, value string,d int) {
    cookie := http.Cookie{
        Name:    name,
        Value:   value,
    }
    if d != 0{
        expires := time.Now().AddDate(0,0,d)
        cookie.Expires = expires
    }
    http.SetCookie(w, &cookie)
}

func getCookie(r *http.Request, name string) string {
    c, err := r.Cookie(name)
    if err != nil {
        return ""
    }
    return c.Value
}

func deleteCookie(w http.ResponseWriter,name string){
    cookie := http.Cookie{
        Name: name,
        MaxAge: -1,
    }
    http.SetCookie(w, &cookie)
}

func signup(w http.ResponseWriter, r *http.Request) {
    switch r.Method {
      case "GET":
        t.ExecuteTemplate(w,"signup.html",nil)
      case "POST":
        r.ParseForm()
        data := r.Form
        var error string
        if data["login"][0] == ""{
            error = "Login can't be empty"
        } else if data["email"][0] == ""{
            error = "Email can't be empty"
        } else if data["password"][0] == ""{
            error = "Password cant't be empty"
        } else if len(data["login"][0]) < 4{
            error = "Login must be at least 4 characters"
        } else if DB.checkLogin(data["login"][0]){
            error = "User with such login already exists"
        } else if !strings.ContainsRune(data["email"][0],'@'){
            error = "Email must contain @"
        } else if DB.checkEmail(data["email"][0]) {
            error = "User with such email already exists"
        } else if len(data["password"][0]) < 8{
            error = "Password must be at least 8 characters"
        } else if data["password2"][0] != data["password"][0]{
            error = "Passwords don't match"
        }
        if error != ""{
            values :=&User{}
            values.Login = data["login"][0]
            values.Email = data["email"][0]
            t, err := template.ParseFiles("signup.html")
            if err != nil{
                http.Error(w,"Internal server error",500)
            }
            t.Execute(w,values)
            fmt.Fprintln(w,"<hr><span style='color:red;'>" + error + "</span>")
        } else {
            hashedPassword, err := bcrypt.GenerateFromPassword([]byte(data["password"][0]),10)
            if err != nil{
                http.Error(w,"Internal server error",500)
            }
            DB.newUser(data["login"][0],data["email"][0],string(hashedPassword))
            http.Redirect(w,r,"/login",http.StatusSeeOther)
        }
    }
}

func signin(w http.ResponseWriter, r *http.Request) {
    switch r.Method {
      case "GET":
        t.ExecuteTemplate(w,"signin.html",nil)
      case "POST":
          r.ParseForm()
          data := r.Form
          var error string
          if !DB.checkLogin(data["login"][0]){
              error = "User with such login doesn't exists"
          } else {
              if !DB.checkPassword(data["login"][0],data["password"][0]){
                  error = "Wrong password"
              }
          }
          if error != ""{
              values :=&User{}
              values.Login = data["login"][0]
              t, err := template.ParseFiles("signin.html")
              if err != nil{
                  http.Error(w,"Internal server error",http.StatusInternalServerError)
              }
              t.Execute(w,values)
              fmt.Fprintln(w,"<hr><span style='color:red;'>" + error + "</span>")
          } else {
              expiresAfter := 0
              if r.FormValue("remember") == "1"{
                  expiresAfter = 30
              }
              token := genToken()
              setCookie(w,"login",data["login"][0],expiresAfter)
              setCookie(w,"session_token",token,expiresAfter)
              DB.newSession(data["login"][0],token)
              http.Redirect(w,r,"/",http.StatusSeeOther)
          }
    }
}

func mainPage(w http.ResponseWriter, r *http.Request) {
    login := getCookie(r,"login")
    token := getCookie(r,"session_token")
    if !DB.checkToken(login,token){
        http.Redirect(w,r,"/login",http.StatusSeeOther)
    }
    user := DB.getUser(login)
    t.ExecuteTemplate(w,"main.html",user)
}

func logout(w http.ResponseWriter,r *http.Request){
    login := getCookie(r,"login")
    token := getCookie(r,"session_token")
    deleteCookie(w,"login")
    deleteCookie(w,"session_token")
    DB.deleteSession(login,token)
    http.Redirect(w,r,"/login",http.StatusSeeOther)
}

func main() {
    http.HandleFunc("/register", signup)
    http.HandleFunc("/login", signin)
    http.HandleFunc("/", mainPage)
    http.HandleFunc("/logout",logout)
    http.ListenAndServe(":8080", nil)
}

Database file:

package main

import (
    "database/sql"
    "golang.org/x/crypto/bcrypt"
    "log"

    _ "github.com/go-sql-driver/mysql"
)

var DB = newDB("root:root574@/signin")

type db struct {
    DB *sql.DB
}

func newDB(name string) *db {
    conn, err := sql.Open("mysql", name)
    if err != nil {
        log.Fatal(err)
    }
    if err = conn.Ping(); err != nil {
        log.Fatal(err)
    }
    return &db{DB: conn}
}

func (db db) newUser(login, email, password string) {
    db.DB.Exec("INSERT INTO users(login,email,password) VALUES (?,?,?)", login, email, password)
}

func (db db) newSession(login, token string) {
    db.DB.Exec("INSERT INTO sessions(login,token) VALUES (?,?)",login,token)
}

func (db db) deleteSession(login, token string) {
    db.DB.Exec("DELETE FROM sessions WHERE login = ? and session_token = ?",login,token)
}

func (db db) checkLogin(login string) bool {
    var rows, _ = db.DB.Query("SELECT id FROM users WHERE login = ?", login)
    if rows.Next() {
        return true
    }
    rows.Close()
    return false
}

func (db db) checkEmail(email string) bool {
    var rows, _ = db.DB.Query("SELECT id FROM users WHERE email = ?", email)
    if rows.Next() {
        return true
    }
    rows.Close()
    return false
}

func (db db) checkPassword(login, password string) bool{
    var rows, _ = db.DB.Query("SELECT password FROM users WHERE login = ?", login)
    var dbpassword string
    rows.Next()
    rows.Scan(&dbpassword)
    rows.Close()
    if bcrypt.CompareHashAndPassword([]byte(dbpassword),[]byte(password)) != nil{
        return false
    }
    return true
}

func (db db) checkToken(login, token string) bool {
    var rows, _ = db.DB.Query("SELECT token FROM sessions WHERE login = ? and token = ?",login,token)
    if rows.Next(){
        return true
    }
    rows.Close()
    return false
}

func (db db) getUser(login string) *User {
    var rows, _ = db.DB.Query("select email FROM users WHERE login = ?",login)
    user := &User{}
    rows.Next()
    rows.Scan(&user.Email)
    rows.Close()
    user.Login = login
    return user
}

Database users table:

+----------+--------------+------+-----+---------+----------------+
| Field    | Type         | Null | Key | Default | Extra          |
+----------+--------------+------+-----+---------+----------------+
| id       | int          | NO   | PRI | NULL    | auto_increment |
| login    | varchar(255) | YES  |     | NULL    |                |
| email    | varchar(255) | YES  |     | NULL    |                |
| password | varchar(255) | YES  |     | NULL    |                |
+----------+--------------+------+-----+---------+----------------+

Database sessions table:

+-------+--------------+------+-----+---------+-------+
| Field | Type         | Null | Key | Default | Extra |
+-------+--------------+------+-----+---------+-------+
| login | varchar(255) | YES  |     | NULL    |       |
| token | varchar(15)  | YES  |     | NULL    |       |
+-------+--------------+------+-----+---------+-------+
\$\endgroup\$
2
  • \$\begingroup\$ I'm looking through main.go now, a couple quick pointers would be to create a global variable t *template.Template and set it equal to ParseFile/ParseGlob in init() so you don't need to call ParseFile 3 separate times. You could just use t.ExecuteTemplate(). Also, pulling that big if/else clause out into a separate validate function using switch would also improve readability \$\endgroup\$ – Coupcoup Aug 25 '20 at 22:34
  • \$\begingroup\$ Please do not update the code in your question to incorporate feedback from answers, doing so goes against the Question + Answer style of Code Review. This is not a forum where you should keep the most updated version in your question. Please see what you may and may not do after receiving answers. \$\endgroup\$ – Mast Aug 28 '20 at 8:37
2
\$\begingroup\$

Never user PSEUDO random values for secrets!

This could be exploited by an attacker by guessing the seed value, in this case:

rand.Seed(time.Now().Unix())

And then generating a possible Cookie value to login.

Instead use cryptographically secure implementations, in golang you should use crypto/rand

\$\endgroup\$
1

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.