faryoung / winsvc Goto Github PK

The eventlog package only accepts event IDs between 1 and 100. This is too 
narrow when not using the EventCreate executable, because the base ID in any 
message file is ORed with the severity and facility numbers to make the final 
ID used with ReportEvent. See, for instance, Microsoft's example here:

http://msdn.microsoft.com/en-us/library/windows/desktop/aa363680(v=vs.85).aspx

Their message file defines MSG_INVALID_COMMAND with Id 0x100, but the Id passed 
to ReportEvent (as seen in the header file) is 0xC0020100.

I suggest just deleting the check in (*Log).report as follows:

diff -r 187091ca1db6 eventlog/log.go
--- a/eventlog/log.go   Sat Jul 21 20:41:30 2012 +1000
+++ b/eventlog/log.go   Wed Oct 02 13:41:25 2013 -0700
@@ -44,10 +44,6 @@
 }

 func (l *Log) report(etype uint16, eid uint32, msg string) error {
-   if eid < 1 || eid > 100 {
-       // EventCreate executable has messages for these only.
-       return errors.New("event id must be between 1 and 1000")
-   }
    ss := []*uint16{syscall.StringToUTF16Ptr(msg)}
    return winapi.ReportEvent(l.Handle, etype, 0, eid, 0, 1, 0, &ss[0], nil)
 }

The winapi and registry aren't really designed for public consumption. I 
recommend we move them into an internal folder to declare them as such.

go version
go version go1.4rc1 windows/amd64

git clone https://github.com/chai2010/gogs to "github.com/gogits/gogs".
Then run these commands:

1. go build
2. sc create gogs binPath= "$(PWD)\gogs.exe web" displayName= "Go Git Service"
3. net start gogs

net start failed, error code 1067.

This could just be my lack of knowledge of Go, but it's worth a shot! :)

I've used https://github.com/kardianos/service/ wrapper for your "winsvc" and 
changed it a bit so dependencies passes to "winsvc" (added missing 
"Dependencies" fields in the structs).

Adding (one) dependency to my application seemed to work, it even shows up in 
properties->dependencies, but when starting the service I got this error:
"The dependency service does not exist or has been marked for deletion."
Which I guess is type mismatch, or similar.

So I looked at the spec 
(https://msdn.microsoft.com/en-us/library/windows/desktop/ms682450(v=vs.85).aspx
) for the "CreateService" function I saw this:

"...A pointer to a double null-terminated array of null-separated names of 
services or load ordering groups that the system must start before this 
service...."

The Config struct in "config.go" for Dependencies is a string and not an 
"double null-terminated array".

My bad?

go version
go version devel +23e48d4f3066 Wed Sep 03 23:10:15 2014 -0400 windows/amd64

go test ./...
?       code.google.com/p/winsvc/debug  [no test files]
ok      code.google.com/p/winsvc/eventlog       0.257s
?       code.google.com/p/winsvc/example        [no test files]
--- FAIL: TestMyService (0.00s)
        mgr_test.go:37: service myservice already exists
FAIL
FAIL    code.google.com/p/winsvc/mgr    0.194s
?       code.google.com/p/winsvc/registry       [no test files]
--- FAIL: TestExample (1.30s)
        svc_test.go:87: Start(myservice) failed: The service did not respond to the start or control request in a timely fashion.
FAIL
FAIL    code.google.com/p/winsvc/svc    1.499s
?       code.google.com/p/winsvc/winapi [no test files]

https://msdn.microsoft.com/en-us/library/ms724911(VS.85).aspx

There are existing various registry functions. Is it possible to support 
fetching registry keys via the above function? I think the difficulty may be in 
supporting the various types 
(https://msdn.microsoft.com/en-us/library/windows/desktop/ms724884%28v=vs.85%29.
aspx).

I'm not sure what a good solution for that would be. Perhaps start out with a 
RegistryGetString function so we have a nice template to work off, then folks 
can add other types as needed?

Running `go test` on a non-Windows machine fails due to missing syscalls. 
Adding "// +build windows" before the package declaration in your two _test.go 
files disables these tests on such machines.

The function svc.IsAnIinteractiveSession (note the extra "i") should be 
svc.IsAnInteractiveSession. Diff:

diff -r 187091ca1db6 svc/security.go
--- a/svc/security.go   Sat Jul 21 20:41:30 2012 +1000
+++ b/svc/security.go   Wed Oct 02 14:01:59 2013 -0700
@@ -50,10 +50,10 @@
    return sid, nil
 }

-// IsAnIinteractiveSession determines if calling process is running 
interactively.
+// IsAnInteractiveSession determines if calling process is running 
interactively.
 // It queries the process token for membership in the Interactive group.
 // http://stackoverflow.com/questions/2668851/how-do-i-detect-that-my-application-is-running-as-service-or-in-an-interactive-s
-func IsAnIinteractiveSession() (bool, error) {
+func IsAnInteractiveSession() (bool, error) {
    interSid, err := allocSid(winapi.SECURITY_INTERACTIVE_RID)
    if err != nil {
        return false, err

from https://groups.google.com/d/msg/golang-nuts/ArQH2CjwXh8/azMGg9VPs-EJ

>>>
The problem manifests as a deadlock between the loop at 
bitbucket.org/kardianos/service/service_windows.go:35 (call this loop K) and 
the loop at code.google.com/p/winsvc/svc/service.go:206 (call this loop B).

If Windows sends multiple service commands to B quickly enough the deadlock can 
be triggered. It occurs like this: B receives a command from Windows and case r 
of its select is taken, so B sends a ChangeRequest to K on an unbuffered 
channel. K receives the command and sends a status update back to B. If, in the 
mean time Windows sends another command to B, then B can select to receive 
either the status update from K or the new command from the OS. If it chooses 
the command it will deadlock because B is blocked sending to K, and K is 
blocked sending to B and neither loop is ready to receive.

The choice of which package to patch depends on how we interpret the 
responsibilities of the two packages as they communicate. But personally I 
think the channel send at B:216 is wrong and needs to be part of the outer 
select statement instead of a stand alone synchronous channel send.
<<<

Alex

Is there a plan to move this repo somewhere else?  If so, when will it happen?

Hi, probably not a common occurrence, but when I use the 
svc.IsAnInteractiveSession as a user in a lot of groups it panics due to the 
size of the syscall.SIDAndAttributes array (svc/security.go:84).

I'm not much of a programmer and i'm even newer with golang so I'm not sure if 
this length can somehow be defined at runtime.  I can fix it by increasing it 
to an arbitary value > gs.GroupCount but obviously this will just fix it for a 
set number of groups returned on the token.

What steps will reproduce the problem?
1. Run code from example folder as a user in a large number of groups 
(gs.GroupCount > 256)

What version of the product are you using? On what operating system?
Windows 7 64 bit, golang 1.3.3 or 1.4.2

Please provide any additional information below.
Trace attached

When calling eventlog.Install or eventlog.InstallAsEventCreate the 
EventMessageFile registry key that gets created should be of type REG_EXPAND_SZ 
instead of REG_SZ so that the %SystemRoot% is expanded to c:\Windows prior to 
being returned.

What is the expected output? What do you see instead?
Because it is using REG_SZ the return value is incorrect, which causes issues 
when you try to access the event message (see 
http://stackoverflow.com/questions/29130586 )

A SetStringExpand method should be added but change this like to use 
REG_EXPAND_SZ 

https://code.google.com/p/winsvc/source/browse/registry/registry.go#64

go version
go version devel +932fe2220746 Sat Sep 20 23:31:11 2014 -0700 windows/amd64

go test ./...
?       code.google.com/p/winsvc/debug  [no test files]
# code.google.com/p/winsvc/example
runtime.pclntab: code.google.com/p/winsvc/svc.servicemain.args_stackmap: not 
defined
runtime.pclntab: code.google.com/p/winsvc/svc.servicectlhandler.args_stackmap: 
not defined
runtime.pclntab: undefined: 
code.google.com/p/winsvc/svc.servicemain.args_stackmap
runtime.pclntab: undefined: 
code.google.com/p/winsvc/svc.servicectlhandler.args_stackmap
ok      code.google.com/p/winsvc/eventlog       0.206s

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.

How hard would it be to support callbacks for watching an event log? I'd like 
to be able to do the example here:

http://msdn.microsoft.com/en-us/library/system.diagnostics.eventlog.entrywritten
(v=vs.110).aspx

with this package. I'm willing to work on this, but was curious how hard you 
think it will be from "just add a few functions to the winapi package" to 
"really hard due to the callback nature of this functionality". In any case, 
it'd be great if I didn't have to write a .net program here.