CVE-2019-16097: Missing Authorization (CWE-862)
value: core/api/user.go in Harbor 1.7.0 through 1.8.2 allows non-admin users to create admin accounts via the POST /api/users API, when Harbor is setup with DB as authentication backend and allow user to do self-registration. Fixed version: v1.7.6 v1.8.3. v.1.9.0. Workaround without applying the fix: configure Harbor to use non-DB authentication backend such as LDAP.
CWE description: The product does not perform an authorization check when an actor attempts to access a resource or perform an action.
CVSS v3 base score: 6.5
Published: 2019-09-08T16:15Z
Repository: https://github.com/goharbor/harbor
Language: Go
Files changed: src/core/api/user.go, src/core/api/user_test.go
Commit date: 2019-08-28 18:30:27 +0800
Commit message: Disallow creating an admin user when registration

This commit enhance the `POST /api/users` API to block request from non-admin to create
admin user.

Signed-off-by: Daniel Jiang <jiangd@vmware.com>

Diff: diff --git a/src/core/api/user.go b/src/core/api/user.go
index f0e852cbe..a58095983 100644
--- a/src/core/api/user.go
+++ b/src/core/api/user.go
@@ -322,10 +322,18 @@ func (ua *UserAPI) Post() {
 	if err != nil {
 		log.Warningf("Bad request in Register: %v", err)
 		ua.RenderError(http.StatusBadRequest, "register error:"+err.Error())
 		return
 	}
+
+	if !ua.IsAdmin && user.HasAdminRole {
+		msg := "Non-admin cannot create an admin user."
+		log.Errorf(msg)
+		ua.SendForbiddenError(errors.New(msg))
+		return
+	}
+
 	userExist, err := dao.UserExists(user, "username")
 	if err != nil {
 		log.Errorf("Error occurred in Register: %v", err)
 		ua.SendInternalServerError(errors.New("internal error"))
 		return
@@ -344,10 +352,11 @@ func (ua *UserAPI) Post() {
 	if emailExist {
 		log.Warning("email has already been used!")
 		ua.SendConflictError(errors.New("email has already been used"))
 		return
 	}
+
 	userID, err := dao.Register(user)
 	if err != nil {
 		log.Errorf("Error occurred in Register: %v", err)
 		ua.SendInternalServerError(errors.New("internal error"))
 		return
diff --git a/src/core/api/user_test.go b/src/core/api/user_test.go
index a666a4d7b..88f35dd0d 100644
--- a/src/core/api/user_test.go
+++ b/src/core/api/user_test.go
@@ -43,71 +43,71 @@ func TestUsersPost(t *testing.T) {
 	apiTest := newHarborAPI()
 	config.Upload(map[string]interface{}{
 		common.AUTHMode: "db_auth",
 	})
 	// case 1: register a new user without admin auth, expect 400, because self registration is on
-	fmt.Println("Register user without admin auth")
+	t.Log("case 1: Register user without admin auth")
 	code, err := apiTest.UsersPost(testUser0002)
 	if err != nil {
 		t.Error("Error occurred while add a test User", err.Error())
 		t.Log(err)
 	} else {
-		assert.Equal(400, code, "Add user status should be 400")
+		assert.Equal(400, code, "case 1: Add user status should be 400")
 	}
 
 	// case 2: register a new user with admin auth, but username is empty, expect 400
-	fmt.Println("Register user with admin auth, but username is empty")
+	t.Log("case 2: Register user with admin auth, but username is empty")
 	code, err = apiTest.UsersPost(testUser0002, *admin)
 	if err != nil {
 		t.Error("Error occurred while add a user", err.Error())
 		t.Log(err)
 	} else {
-		assert.Equal(400, code, "Add user status should be 400")
+		assert.Equal(400, code, "case 2: Add user status should be 400")
 	}
 
 	// case 3: register a new user with admin auth, but bad username format, expect 400
 	testUser0002.Username = "test@$"
-	fmt.Println("Register user with admin auth, but bad username format")
+	t.Log("case 3: Register user with admin auth, but bad username format")
 	code, err = apiTest.UsersPost(testUser0002, *admin)
 	if err != nil {
 		t.Error("Error occurred while add a user", err.Error())
 		t.Log(err)
 	} else {
-		assert.Equal(400, code, "Add user status should be 400")
+		assert.Equal(400, code, "case 3: Add user status should be 400")
 	}
 
 	// case 4: register a new user with admin auth, but bad userpassword format, expect 400
 	testUser0002.Username = "testUser0002"
-	fmt.Println("Register user with admin auth, but empty password.")
+	t.Log("case 4: Register user with admin auth, but empty password.")
 	code, err = apiTest.UsersPost(testUser0002, *admin)
 	if err != nil {
 		t.Error("Error occurred while add a user", err.Error())
 		t.Log(err)
 	} else {
-		assert.Equal(400, code, "Add user status should be 400")
+		assert.Equal(400, code, "case 4: Add user status should be 400")
 	}
 
 	// case 5: register a new user with admin auth, but email is empty, expect 400
 	testUser0002.Password = "testUser0002"
-	fmt.Println("Register user with admin auth, but email is empty")
+	t.Log("case 5: Register user with admin auth, but email is empty")
 	code, err = apiTest.UsersPost(testUser0002, *admin)
 	if err != nil {
 		t.Error("Error occurred while add a user", err.Error())
 		t.Log(err)
 	} else {
-		assert.Equal(400, code, "Add user status should be 400")
+		assert.Equal(400, code, "case 5: Add user status should be 400")
 	}
 
 	// case 6: register a new user with admin auth, but bad email format, expect 400
 	testUser0002.Email = "test..."
-	fmt.Println("Register user with admin auth, but bad email format")
+	t.Log("case 6: Register user with admin auth, but bad email format")
 	code, err = apiTest.UsersPost(testUser0002, *admin)
 	if err != nil {
 		t.Error("Error occurred while add a user", err.Error())
 		t.Log(err)
 	} else {
-		assert.Equal(400, code, "Add user status should be 400")
+		assert.Equal(400, code, "case 6: Add user status should be 400")
 	}
 
 	// case 7: register a new user with admin auth, but userrealname is empty, expect 400
 	/*
 		testUser0002.Email = "testUser0002@mydomain.com"
@@ -121,63 +121,75 @@ func TestUsersPost(t *testing.T) {
 		}
 	*/
 	// case 8: register a new user with admin auth, but bad userrealname format, expect 400
 	testUser0002.Email = "testUser0002@mydomain.com"
 	testUser0002.Realname = "test$com"
-	fmt.Println("Register user with admin auth, but bad user realname format")
+	t.Log("case 8: Register user with admin auth, but bad user realname format")
 	code, err = apiTest.UsersPost(testUser0002, *admin)
 	if err != nil {
 		t.Error("Error occurred while add a user", err.Error())
 		t.Log(err)
 
 	} else {
-		assert.Equal(400, code, "Add user status should be 400")
+		assert.Equal(400, code, "case 8: Add user status should be 400")
 	}
 
 	// case 9: register a new user with admin auth, but bad user comment, expect 400
 	testUser0002.Realname = "testUser0002"
 	testUser0002.Comment = "vmmmmmmmmmmmmmmmmmmmmmmmmmmmmmmmmmmmmmmmmmmmmmmmmmmmmmmmmmm"
-	fmt.Println("Register user with admin auth, but user comment length is illegal")
+	t.Log("case 9: Register user with admin auth, but user comment length is illegal")
 	code, err = apiTest.UsersPost(testUser0002, *admin)
 	if err != nil {
 		t.Error("Error occurred while add a user", err.Error())
 		t.Log(err)
 	} else {
-		assert.Equal(400, code, "Add user status should be 400")
+		assert.Equal(400, code, "case 9: Add user status should be 400")
 	}
-
-	// case 10: register a new user with admin auth, expect 201
-	fmt.Println("Register user with admin auth, right parameters")
 	testUser0002.Comment = "test user"
+
+	// case 10: register an admin using non-admin user, expect 403
+	t.Log("case 10: Register admin user with non admin auth")
+	testUser0002.HasAdminRole = true
+	code, err = apiTest.UsersPost(testUser0002)
+	if err != nil {
+		t.Error("Error occurred while add a user", err.Error())
+		t.Log(err)
+	} else {
+		assert.Equal(http.StatusForbidden, code, "case 10: Add user status should be 403")
+	}
+	testUser0002.HasAdminRole = false
+
+	// case 11: register a new user with admin auth, expect 201
+	t.Log("case 11: Register user with admin auth, right parameters")
 	code, err = apiTest.UsersPost(testUser0002, *admin)
 	if err != nil {
 		t.Error("Error occurred while add a user", err.Error())
 		t.Log(err)
 	} else {
-		assert.Equal(201, code, "Add user status should be 201")
+		assert.Equal(201, code, "case 11: Add user status should be 201")
 	}
 
-	// case 11: register duplicate user with admin auth, expect 409
-	fmt.Println("Register duplicate user with admin auth")
+	// case 12: register duplicate user with admin auth, expect 409
+	t.Log("case 12: Register duplicate user with admin auth")
 	code, err = apiTest.UsersPost(testUser0002, *admin)
 	if err != nil {
 		t.Error("Error occurred while add a user", err.Error())
 		t.Log(err)
 	} else {
-		assert.Equal(409, code, "Add user status should be 409")
+		assert.Equal(409, code, "case 12: Add user status should be 409")
 	}
 
-	// case 12: register a new user with admin auth, but duplicate email, expect 409
-	fmt.Println("Register user with admin auth, but duplicate email")
+	// case 13: register a new user with admin auth, but duplicate email, expect 409
+	t.Log("case 13: Register user with admin auth, but duplicate email")
 	testUser0002.Username = "testUsertest"
 	testUser0002.Email = "testUser0002@mydomain.com"
 	code, err = apiTest.UsersPost(testUser0002, *admin)
 	if err != nil {
 		t.Error("Error occurred while add a user", err.Error())
 		t.Log(err)
 	} else {
-		assert.Equal(409, code, "Add user status should be 409")
+		assert.Equal(409, code, "case 13: Add user status should be 409")
 	}
 }
 
 func TestUsersGet(t *testing.T) {
 

Vulnerable code: 	fmt.Println("Register user without admin auth")
		assert.Equal(400, code, "Add user status should be 400")
	fmt.Println("Register user with admin auth, but username is empty")
		assert.Equal(400, code, "Add user status should be 400")
	fmt.Println("Register user with admin auth, but bad username format")
		assert.Equal(400, code, "Add user status should be 400")
	fmt.Println("Register user with admin auth, but empty password.")
		assert.Equal(400, code, "Add user status should be 400")
	fmt.Println("Register user with admin auth, but email is empty")
		assert.Equal(400, code, "Add user status should be 400")
	fmt.Println("Register user with admin auth, but bad email format")
		assert.Equal(400, code, "Add user status should be 400")
	fmt.Println("Register user with admin auth, but bad user realname format")
		assert.Equal(400, code, "Add user status should be 400")
	fmt.Println("Register user with admin auth, but user comment length is illegal")
		assert.Equal(400, code, "Add user status should be 400")

	// case 10: register a new user with admin auth, expect 201
	fmt.Println("Register user with admin auth, right parameters")
		assert.Equal(201, code, "Add user status should be 201")
	// case 11: register duplicate user with admin auth, expect 409
	fmt.Println("Register duplicate user with admin auth")
		assert.Equal(409, code, "Add user status should be 409")
	// case 12: register a new user with admin auth, but duplicate email, expect 409
	fmt.Println("Register user with admin auth, but duplicate email")
		assert.Equal(409, code, "Add user status should be 409")
Fixed code: 
	if !ua.IsAdmin && user.HasAdminRole {
		msg := "Non-admin cannot create an admin user."
		log.Errorf(msg)
		ua.SendForbiddenError(errors.New(msg))
		return
	}


	t.Log("case 1: Register user without admin auth")
		assert.Equal(400, code, "case 1: Add user status should be 400")
	t.Log("case 2: Register user with admin auth, but username is empty")
		assert.Equal(400, code, "case 2: Add user status should be 400")
	t.Log("case 3: Register user with admin auth, but bad username format")
		assert.Equal(400, code, "case 3: Add user status should be 400")
	t.Log("case 4: Register user with admin auth, but empty password.")
		assert.Equal(400, code, "case 4: Add user status should be 400")
	t.Log("case 5: Register user with admin auth, but email is empty")
		assert.Equal(400, code, "case 5: Add user status should be 400")
	t.Log("case 6: Register user with admin auth, but bad email format")
		assert.Equal(400, code, "case 6: Add user status should be 400")
	t.Log("case 8: Register user with admin auth, but bad user realname format")
		assert.Equal(400, code, "case 8: Add user status should be 400")
	t.Log("case 9: Register user with admin auth, but user comment length is illegal")
		assert.Equal(400, code, "case 9: Add user status should be 400")

	// case 10: register an admin using non-admin user, expect 403
	t.Log("case 10: Register admin user with non admin auth")
	testUser0002.HasAdminRole = true
	code, err = apiTest.UsersPost(testUser0002)
	if err != nil {
		t.Error("Error occurred while add a user", err.Error())
		t.Log(err)
	} else {
		assert.Equal(http.StatusForbidden, code, "case 10: Add user status should be 403")
	}
	testUser0002.HasAdminRole = false

	// case 11: register a new user with admin auth, expect 201
	t.Log("case 11: Register user with admin auth, right parameters")
		assert.Equal(201, code, "case 11: Add user status should be 201")
	// case 12: register duplicate user with admin auth, expect 409
	t.Log("case 12: Register duplicate user with admin auth")
		assert.Equal(409, code, "case 12: Add user status should be 409")
	// case 13: register a new user with admin auth, but duplicate email, expect 409
	t.Log("case 13: Register user with admin auth, but duplicate email")
		assert.Equal(409, code, "case 13: Add user status should be 409")