- Download Diff

Summary:

Deny invalid path

Review Request #2422 — Created April 4, 2023 and submitted April 13, 2023, 12:06 a.m.

Information
Owner:	aklitzing@gmail.com
Repository:	grim/hgkeeper
Branch:	default
Bugs:
Depends On:	~~2421~~
Blocks:	2424
Reviewers
Groups:
People:	grim

Description

If an authenticated user calls hg init hg.host.com/dummy/../../../etc
it will create the repository in another root directory if the process of
hgkeeper has permissions for this.
This could be an attack to the server.

Also hgkeeper admin repository can be overriden like this.
hg init ssh://hg.host.com/dummy/../hgkeeper/keys

Testing Done

Commits

Summary		Author
	Deny "hg init" for invalid path	Andre Klitzing

Screenshots

Issues

Description	From	Last Updated
mh, that is a bad fix... let's retry...	aklitzing@gmail.com	April 4, 2023, 5:01 p.m.
we probably want to use filepath.Clean here on repoName at a minimum.	grim	April 10, 2023, 5:42 p.m.
not really a fan of a security related function being duplicated. I know this is because there's no good place …	grim	April 12, 2023, 5:49 p.m.

Commits:

	Summary		Author
-		Deny "hg init" for invalid path	Andre Klitzing
+		Deny "hg init" for invalid path	Andre Klitzing

Diff:

Revision 2 (+10)

Show changes

ssh/commands/init.go

Description:

~		If an authenticated user calls "hg init hg.host.com/dummy/../../../etc"
	~	If an authenticated user calls `hg init hg.host.com/dummy/../../../etc`
		it will create the repository in another root directory if the process of
		hgkeeper has permissions for this.
		This could be an attack to the server.
	+
	+	Also hgkeeper admin repository can be overriden like this.
	+	`hg init ssh://hg.host.com/dummy/../hgkeeper/keys`

In /r/2421/diff/1/#index_header I check if the repository is known.
That should be checked here, too. It will always be a problem if someone creates a new repository in a sub-directory of another repository.

Testing Done:

Container tested only... maybe "once/command.go" need to be adjusted, too.

Show all issues
```
mh, that is a bad fix... let's retry...
```

Summary:

-	Deny "hg init" for invalid path
+	Deny invalid path

Commits:

	Summary		Author
-		Deny "hg init" for invalid path	Andre Klitzing
+		Deny "hg init" for invalid path	Andre Klitzing

Diff:

Revision 3 (+38 -8)

Show changes

	ssh/commands/commands.go
	ssh/commands/init.go
	ssh/commands/serve.go

Commits:

	Summary		Author
-		Deny "hg init" for invalid path	Andre Klitzing
+		Deny "hg init" for invalid path	Andre Klitzing

Diff:

Revision 4 (+38 -16)

Show changes

	access/access.go
	ssh/commands/commands.go
	ssh/commands/init.go
	ssh/commands/serve.go

Commits:

	Summary		Author
-		Deny "hg init" for invalid path	Andre Klitzing
+		Deny "hg init" for invalid path	Andre Klitzing

Diff:

Revision 5 (+38 -8)

Show changes

	ssh/commands/commands.go
	ssh/commands/init.go
	ssh/commands/serve.go

ssh/commands/commands.go (Diff revision 5)

Show all issues

we probably want to use filepath.Clean here on repoName at a minimum.

Commits:

	Summary		Author
-		Deny "hg init" for invalid path	Andre Klitzing
+		Deny "hg init" for invalid path	Andre Klitzing

Diff:

Revision 6 (+40 -8)

Show changes

	ssh/commands/commands.go
	ssh/commands/init.go
	ssh/commands/serve.go

Commits:

	Summary		Author
-		Deny "hg init" for invalid path	Andre Klitzing
+		Deny "hg init" for invalid path	Andre Klitzing

Diff:

Revision 7 (+38 -8)

Show changes

	ssh/commands/commands.go
	ssh/commands/init.go
	ssh/commands/serve.go

Commits:

	Summary		Author
-		Deny "hg init" for invalid path	Andre Klitzing
+		Deny "hg init" for invalid path	Andre Klitzing

Diff:

Revision 8 (+70 -12)

Show changes

	once/command.go
	ssh/commands/commands.go
	ssh/commands/init.go
	ssh/commands/serve.go

Testing Done:

Container tested only... maybe "once/command.go" need to be adjusted, too.

Commits:

	Summary		Author
-		Deny "hg init" for invalid path	Andre Klitzing
+		Deny "hg init" for invalid path	Andre Klitzing

Diff:

Revision 9 (+70 -12)

Show changes

	once/command.go
	ssh/commands/commands.go
	ssh/commands/init.go
	ssh/commands/serve.go

ssh/commands/commands.go (Diff revision 9)

Show all issues

not really a fan of a security related function being duplicated. I know this is because there's no good place to put it right now. Maybe a normalize package?

aklitzing@gmail.com April 12, 2023, 5:49 p.m.

Yeah, I didn't like it, too. I moved it to new repositories.go as a normal func instead of another lambda call. So it uses the normal "canInit/canRead/canRemove" for permission which will fail for empty/invalid path.

Commits:

	Summary		Author
-		Deny "hg init" for invalid path	Andre Klitzing
+		Deny "hg init" for invalid path	Andre Klitzing

Depends On:

+	~~2421 - Introduce "rm <repo>" command~~

Diff:

Revision 10 (+40 -10)

Show changes

	access/repositories.go
	once/command.go
	ssh/commands/commands.go

Commits:

	Summary		Author
-		Deny "hg init" for invalid path	Andre Klitzing
+		Deny "hg init" for invalid path	Andre Klitzing

Diff:

Revision 11 (+46 -10)

Show changes

	access/access.go
	access/repositories.go
	once/command.go
	ssh/commands/commands.go

Ship it!

```
Ship It!
```
```
Nice work, thanks!!
```

You have a pending review.

Review Board 5.0.4

Deny invalid path

Commits:

Diff:

Description:

Testing Done:

Summary:

Commits:

Diff:

Commits:

Diff:

Commits:

Diff:

Commits:

Diff:

Commits:

Diff:

Commits:

Diff:

Testing Done:

Commits:

Diff:

Commits:

Depends On:

Diff:

Commits:

Diff:

Status: Closed (submitted)