Fix leaking existence of username from error

This commit is contained in:
Pierre Fersing 2021-02-13 15:00:35 +01:00
parent f1a3fef9ee
commit 90a24b52c6
9 changed files with 139 additions and 0 deletions

View File

@ -88,11 +88,23 @@ func TestFiles(t *testing.T) {
So(authenticated, ShouldBeFalse)
})
Convey("Given a wrong username, it should not authenticate it and not return error", func() {
authenticated, err := files.GetUser(user4, "whatever_password", "")
So(err, ShouldBeNil)
So(authenticated, ShouldBeFalse)
})
//There are no superusers for files
Convey("For any user superuser should return false", func() {
superuser, err := files.GetSuperuser(user1)
So(err, ShouldBeNil)
So(superuser, ShouldBeFalse)
Convey("Including non-present username", func() {
superuser, err := files.GetSuperuser(user4)
So(err, ShouldBeNil)
So(superuser, ShouldBeFalse)
})
})
testTopic1 := `test/topic/1`

View File

@ -136,6 +136,11 @@ func (o Mongo) GetUser(username, password, clientid string) (bool, error) {
err := uc.FindOne(context.TODO(), bson.M{"username": username}).Decode(&user)
if err != nil {
if err == mongo.ErrNoDocuments {
// avoid leaking the fact that user exists or not though error.
return false, nil
}
log.Debugf("Mongo get user error: %s", err)
return false, err
}
@ -161,6 +166,11 @@ func (o Mongo) GetSuperuser(username string) (bool, error) {
err := uc.FindOne(context.TODO(), bson.M{"username": username}).Decode(&user)
if err != nil {
if err == mongo.ErrNoDocuments {
// avoid leaking the fact that user exists or not though error.
return false, nil
}
log.Debugf("Mongo get superuser error: %s", err)
return false, err
}
@ -179,6 +189,11 @@ func (o Mongo) CheckAcl(username, topic, clientid string, acc int32) (bool, erro
err := uc.FindOne(context.TODO(), bson.M{"username": username}).Decode(&user)
if err != nil {
if err == mongo.ErrNoDocuments {
// avoid leaking the fact that user exists or not though error.
return false, nil
}
log.Debugf("Mongo get superuser error: %s", err)
return false, err
}

View File

@ -32,6 +32,7 @@ func TestMongoRaw(t *testing.T) {
const username2 = "test2"
const userPass2 = "testpw"
const userPassHash2 = "PBKDF2$sha512$100000$os24lcPr9cJt2QDVWssblQ==$dEOwgFUoMNt+Q8FHWXl03pZTg/RY47JdSTAx/KjhYKpbugOYg1WWG0tW0V2aqBnSCDLYJdRrkNf3p/PUoKLvkA=="
const wrongUsername = "not_present"
//Define Common Mongo Configuration
var authOpts = make(map[string]string)
@ -90,6 +91,11 @@ func TestMongoRaw(t *testing.T) {
So(err, ShouldBeNil)
So(authenticated, ShouldBeFalse)
})
Convey("Given wrongusername, it should not authenticate it and don't return error", func() {
authenticated, err := mongo.GetUser(wrongUsername, "whatever_password", "")
So(err, ShouldBeNil)
So(authenticated, ShouldBeFalse)
})
Convey("Given username1 that is superuser, super user check should pass", func() {
superuser, err := mongo.GetSuperuser(username1)
So(err, ShouldBeNil)
@ -101,6 +107,11 @@ func TestMongoRaw(t *testing.T) {
So(superuser, ShouldBeFalse)
})
})
Convey("Given wrongusername, super check should no pass and don't return error", func() {
authenticated, err := mongo.GetSuperuser(wrongUsername)
So(err, ShouldBeNil)
So(authenticated, ShouldBeFalse)
})
Convey("Given correct username2 password, but using wrong salt format, user should not authenticate", func() {
authenticated, err := mongo.GetUser(username2, userPass2, "")
So(err, ShouldBeNil)
@ -179,6 +190,12 @@ func TestMongoRaw(t *testing.T) {
So(tt1, ShouldBeTrue)
So(tt2, ShouldBeTrue)
})
Convey("Given a bad username, acl check should not return error", func() {
testTopic1 := `test/topic/1`
tt1, err1 := mongo.CheckAcl(wrongUsername, testTopic1, clientID, MOSQ_ACL_READ)
So(err1, ShouldBeNil)
So(tt1, ShouldBeFalse)
})
mongoDb.Drop(context.TODO())
mongo.Halt()

View File

@ -221,6 +221,11 @@ func (o Mysql) GetUser(username, password, clientid string) (bool, error) {
err := o.DB.Get(&pwHash, o.UserQuery, username)
if err != nil {
if err == sql.ErrNoRows {
// avoid leaking the fact that user exists or not though error.
return false, nil
}
log.Debugf("MySql get user error: %s", err)
return false, err
}
@ -250,6 +255,11 @@ func (o Mysql) GetSuperuser(username string) (bool, error) {
err := o.DB.Get(&count, o.SuperuserQuery, username)
if err != nil {
if err == sql.ErrNoRows {
// avoid leaking the fact that user exists or not though error.
return false, nil
}
log.Debugf("MySql get superuser error: %s", err)
return false, err
}

View File

@ -43,6 +43,7 @@ func TestMysql(t *testing.T) {
userPass := "testpw"
//Hash generated by the pw utility
userPassHash := "PBKDF2$sha512$100000$os24lcPr9cJt2QDVWssblQ==$BK1BQ2wbwU1zNxv3Ml3wLuu5//hPop3/LvaPYjjCwdBvnpwusnukJPpcXQzyyjOlZdieXTx6sXAcX4WnZRZZnw=="
wrongUsername := "not_present"
insertQuery := "INSERT INTO test_user(username, password_hash, is_admin) values(?, ?, ?)"
@ -72,12 +73,26 @@ func TestMysql(t *testing.T) {
})
Convey("Given a wrong username, it should not authenticate it and not return error", func() {
authenticated, err := mysql.GetUser(wrongUsername, "whatever_password", "")
So(err, ShouldBeNil)
So(authenticated, ShouldBeFalse)
})
Convey("Given a username that is admin, super user should pass", func() {
superuser, err := mysql.GetSuperuser(username)
So(err, ShouldBeNil)
So(superuser, ShouldBeTrue)
})
Convey("Given a wrong username, super user should not return error", func() {
superuser, err := mysql.GetSuperuser(wrongUsername)
So(err, ShouldBeNil)
So(superuser, ShouldBeFalse)
})
//Now create some acls and test topics
strictAcl := "test/topic/1"
@ -176,6 +191,13 @@ func TestMysql(t *testing.T) {
So(tt1, ShouldBeTrue)
})
Convey("Given a bad username, acl check should not return error", func() {
testTopic1 := `test/topic/1`
tt1, err1 := mysql.CheckAcl(wrongUsername, testTopic1, clientID, MOSQ_ACL_READ)
So(err1, ShouldBeNil)
So(tt1, ShouldBeFalse)
})
//Empty db
mysql.DB.MustExec("delete from test_user where 1 = 1")
mysql.DB.MustExec("delete from test_acl where 1 = 1")

View File

@ -166,6 +166,11 @@ func (o Postgres) GetUser(username, password, clientid string) (bool, error) {
err := o.DB.Get(&pwHash, o.UserQuery, username)
if err != nil {
if err == sql.ErrNoRows {
// avoid leaking the fact that user exists or not though error.
return false, nil
}
log.Debugf("PG get user error: %s", err)
return false, err
}
@ -195,6 +200,11 @@ func (o Postgres) GetSuperuser(username string) (bool, error) {
err := o.DB.Get(&count, o.SuperuserQuery, username)
if err != nil {
if err == sql.ErrNoRows {
// avoid leaking the fact that user exists or not though error.
return false, nil
}
log.Debugf("PG get superuser error: %s", err)
return false, err
}

View File

@ -41,6 +41,7 @@ func TestPostgres(t *testing.T) {
userPass := "testpw"
//Hash generated by the pw utility
userPassHash := "PBKDF2$sha512$100000$os24lcPr9cJt2QDVWssblQ==$BK1BQ2wbwU1zNxv3Ml3wLuu5//hPop3/LvaPYjjCwdBvnpwusnukJPpcXQzyyjOlZdieXTx6sXAcX4WnZRZZnw=="
wrongUsername := "not_present"
insertQuery := "INSERT INTO test_user(username, password_hash, is_admin) values($1, $2, $3) returning id"
@ -67,12 +68,26 @@ func TestPostgres(t *testing.T) {
})
Convey("Given a wrong username, it should not authenticate it and not return error", func() {
authenticated, err := postgres.GetUser(wrongUsername, "whatever_password", "")
So(err, ShouldBeNil)
So(authenticated, ShouldBeFalse)
})
Convey("Given a username that is admin, super user should pass", func() {
superuser, err := postgres.GetSuperuser(username)
So(err, ShouldBeNil)
So(superuser, ShouldBeTrue)
})
Convey("Given a wrong username, super user should not return error", func() {
superuser, err := postgres.GetSuperuser(wrongUsername)
So(err, ShouldBeNil)
So(superuser, ShouldBeFalse)
})
//Now create some acls and test topics
strictAcl := "test/topic/1"
@ -167,6 +182,13 @@ func TestPostgres(t *testing.T) {
So(tt1, ShouldBeTrue)
})
Convey("Given a bad username, acl check should not return error", func() {
testTopic1 := `test/topic/1`
tt1, err1 := postgres.CheckAcl(wrongUsername, testTopic1, clientID, MOSQ_ACL_READ)
So(err1, ShouldBeNil)
So(tt1, ShouldBeFalse)
})
//Empty db
postgres.DB.MustExec("delete from test_user where 1 = 1")
postgres.DB.MustExec("delete from test_acl where 1 = 1")

View File

@ -100,6 +100,11 @@ func (o Sqlite) GetUser(username, password, clientid string) (bool, error) {
err := o.DB.Get(&pwHash, o.UserQuery, username)
if err != nil {
if err == sql.ErrNoRows {
// avoid leaking the fact that user exists or not though error.
return false, nil
}
log.Debugf("SQlite get user error: %s", err)
return false, err
}
@ -129,6 +134,11 @@ func (o Sqlite) GetSuperuser(username string) (bool, error) {
err := o.DB.Get(&count, o.SuperuserQuery, username)
if err != nil {
if err == sql.ErrNoRows {
// avoid leaking the fact that user exists or not though error.
return false, nil
}
log.Debugf("sqlite get superuser error: %s", err)
return false, err
}

View File

@ -73,6 +73,8 @@ func TestFileSqlite(t *testing.T) {
//Hash generated by the pw utility
userPassHash := "PBKDF2$sha512$100000$os24lcPr9cJt2QDVWssblQ==$BK1BQ2wbwU1zNxv3Ml3wLuu5//hPop3/LvaPYjjCwdBvnpwusnukJPpcXQzyyjOlZdieXTx6sXAcX4WnZRZZnw=="
wrongUsername := "not_present"
insertQuery := "INSERT INTO test_user(username, password_hash, is_admin) values(?, ?, ?)"
userID := int64(0)
@ -101,11 +103,24 @@ func TestFileSqlite(t *testing.T) {
})
Convey("Given wrongusername, it should not authenticate it and don't return error", func() {
authenticated, err := sqlite.GetUser(wrongUsername, "whatever_password", "")
So(err, ShouldBeNil)
So(authenticated, ShouldBeFalse)
})
Convey("Given a username that is admin, super user should pass", func() {
superuser, err := sqlite.GetSuperuser(username)
So(err, ShouldBeNil)
So(superuser, ShouldBeTrue)
})
Convey("Given wrongusername, super check should no pass and don't return error", func() {
authenticated, err := sqlite.GetSuperuser(wrongUsername)
So(err, ShouldBeNil)
So(authenticated, ShouldBeFalse)
})
//Now create some acls and test topics
@ -204,6 +219,12 @@ func TestFileSqlite(t *testing.T) {
So(tt1, ShouldBeTrue)
})
Convey("Given a bad username, acl check should not return error", func() {
tt1, err1 := sqlite.CheckAcl(wrongUsername, "test/test", clientID, MOSQ_ACL_READ)
So(err1, ShouldBeNil)
So(tt1, ShouldBeFalse)
})
//Empty db
sqlite.DB.MustExec("delete from test_user where 1 = 1")
sqlite.DB.MustExec("delete from test_acl where 1 = 1")