CreateService doesn't wrap path to executable in quotes leading to privilege escalation exploit

[GoogleCodeExporter]

Using an unquoted path to a service executable can allow an attacker to create 
c:\program.exe that can be used to elevate local privileges (see 
http://cwe.mitre.org/data/definitions/428.html ).

What steps will reproduce the problem?
1. copy http://bosun.org/scollector/ to c:\Program 
Files\scollector\scollector.exe
2. run scollector.exe -winsvc="install" to install it as a service
3. run sc.exe qc scollector to see the path used to start the service
4. (cleanup) run scollector.exe -winsvc="remove" to uninstall the service

What is the expected output? What do you see instead?
Expected 1: (when there are no additional arguments)
BINARY_PATH_NAME   : "C:\Program Files\scollector\scollector.exe"
or
Expected 2: (when using additional arguments)
BINARY_PATH_NAME   : "C:\Program Files\scollector\scollector.exe" -h 
bosunhostname

Actual result:
BINARY_PATH_NAME   : C:\Program Files\scollector\scollector.exe

Confirmed workaround: when calling the CreateService function, provide the 
exepath wrapped in quotes:
s, err = m.CreateService(name, fmt.sprintf(`"%s"`, exepath), ...)
or when specifying additional arguments:
s, err = m.CreateService(name, fmt.sprintf(`"%s" -h %s`, exepath, 
bosunhostname), ...)

Please provide any additional information below.
See http://www.commonexploits.com/unquoted-service-paths/ for more details 
about unquoted service path exploit.

Not sure if this should be part of the winsvc library or not (sc.exe create 
scollector binPath= "c:\program files\test.exe" does the same thing), but at 
the minimum it should be included in the documentation to warn about making 
sure paths are quoted.

Ideally the CreateService method at 
https://code.google.com/p/winsvc/source/browse/mgr/mgr.go#78 would check to see 
if exepath is a path to a file that currently exists, and if so it would wrap 
it in quotes to fix the majority of cases. This wouldn't address the additional 
arguments case, but not sure there is a reliable way to detect that.

I guess another approach would be to require exepath to always start with a " 
to ensure that the path to the executable is quoted.

Original issue reported on code.google.com by gb...@stackoverflow.com on 19 Apr 2015 at 9:56

[GoogleCodeExporter]

I always avoid paths with space in it. So this does not affect me. But I can 
see the problem.

Luckily, I am moving this package, so we can change package API. I have created 
CL for new package https://go-review.googlesource.com/#/c/9104. And 
(surprisingly) someone raised issue similar to yours just this morning 
https://go-review.googlesource.com/#/c/9104/1/windows/svc/mgr/mgr.go@79. I have 
a proposal for both issues there. Feel free to comment at the new place, if you 
like.

Thank you.

Alex

Original comment by alex.bra...@gmail.com on 20 Apr 2015 at 7:59

• Changed state: Accepted

[GoogleCodeExporter]

Excellent. I heard things might be moving and wanted to make sure this was a 
known issue. I see your comments in 
https://go-review.googlesource.com/#/c/9104/1..2/windows/svc/mgr/mgr.go 

The new method signature for CreateService that allows args parameter and the 
call to syscall.EscapeArg(exepath) looks like a great solution.

Keep up the great work!

Original comment by gb...@stackoverflow.com on 21 Apr 2015 at 11:07

[GoogleCodeExporter]

Just submitted https://go-review.googlesource.com/#/c/9104 I consider this 
issue closed.

Alex

Original comment by alex.bra...@gmail.com on 1 May 2015 at 5:40

[GoogleCodeExporter]

Original comment by alex.bra...@gmail.com on 1 May 2015 at 5:41

• Changed state: Fixed