Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

crypto/rand: rand.Read will fail on Windows if you os.ClearEnv() #8002

Closed
gopherbot opened this issue May 15, 2014 · 9 comments
Closed

crypto/rand: rand.Read will fail on Windows if you os.ClearEnv() #8002

gopherbot opened this issue May 15, 2014 · 9 comments

Comments

@gopherbot
Copy link

by harvey@countersoft.com:

On Windows os.ClearEnv() nukes %SystemRoot% which usually points to
"C:\Windows".

This kills rand.Read as it cannot find Microsoft Crypto 2.0 API dynamic link library.

Thus os.ClearEnv() should leave %SystemRoot% intact on Windows?
@davecheney
Copy link
Contributor

Comment 1:

Can I ask two background questions:
1. why are you using os.ClearEnv(), what does clearing the environment give you? Maybe
there is another way to achieve the same result.
2. Rather than not clearing %SYSTEMROOT%, maybe we can fix rand.Read on _windows to
store the the environment variables it needs in another place. Maybe, i'm not sure. On
unix os.ClearEnv() clears a *copy* of the environment variables, the original ones exist
in their original location, but are ofcourse inaccessible to Go code.

@gopherbot
Copy link
Author

Comment 2 by harvey@countersoft.com:

Dave,
The problem came to light during testing when some code cleared the environment. Not
production code.
Just not sure that %SystemRoot% should ever be nuked...

@bradfitz
Copy link
Contributor

Comment 3:

Labels changed: added os-windows, repo-main.

Status changed to Accepted.

@alexbrainman
Copy link
Member

Comment 5:

harvey,
Do you have a small program to demonstrate? Thank you.
Alex

@alexbrainman
Copy link
Member

Comment 6:

Cannot reproduce:
U:\>type main.go
package main
import (
        "crypto/rand"
        "fmt"
        "log"
        "os"
)
func printRand() error {
        b := make([]byte, 5)
        _, err := rand.Read(b)
        if err != nil {
                return err
        }
        fmt.Printf("%v\n", b)
        return nil
}
func main() {
        os.Clearenv()
        err := printRand()
        if err != nil {
                log.Fatalln(err)
        }
}
U:\>go run main.go
[51 122 78 42 65]
U:\>hg id -R %GOROOT%
263c466e5491 tip
U:\>
Closing as TimedOut. Will reopen if more information is available.
Alex

Status changed to TimedOut.

@gopherbot
Copy link
Author

Comment 7 by saar@countersoft.com:

Try this:
package main
import (
    "crypto/rand"
    "fmt"
    "os"
)
type UUID [16]byte
func main() {
    os.Clearenv()
    u := new(UUID)
    _, err := rand.Read(u[:])
    if err != nil {
        fmt.Println(err)
        return
    }
    fmt.Println(u)
}

@alexbrainman
Copy link
Member

Comment 8:

saar@countersoft.com,
Thank you for the program. Your program is similar to mine in #6. And your program
succeeds on my computer (windows/386). But it does fails on windows/amd64 (I didn't
think to try it there before) with "CryptAcquireContext: Invalid Signature.". So I can
reproduce your issue on windows/amd64.
Unfortunately, I don't see what we can do here. You suggest to leave SYSTEMROOT variable
alone. But what about other variables? What else can break when some environmental
variable  is missing? We would have to document that behaviour (not "clearing"
SYSTEMROOT in os.Clearenv on windows). We would need some "official" recommendation to
leave SYSTEMROOT alone. I haven't found any. Have you? Starting new process without
SYSTEMROOT listed would have similar effect. Should we prohibit that too, and silently
add SYSTEMROOT variable?
I think there are too many questions with your suggestion. I think most sensible thing
is to leave everything as is, and rely on us programmes doing our job properly.
> May 15 (4 days ago) Project Member #1 dave@cheney.net
> ... 
> 2. Rather than not clearing %SYSTEMROOT%, maybe we can fix rand.Read on _windows to
store the the environment variables it needs in another place. Maybe, i'm not sure. On
unix os.ClearEnv() clears a *copy* of the environment variables, the original ones exist
in their original location, but are ofcourse inaccessible to Go code.
syscall.Clearenv does not clear process environment. But it has this comment "//
TODO(bradfitz): pass through to C". So I think it is something that still to happen (if
anyone cares about it). So I think windows version is more correct here. Mind you
windows syscall.Clearenv is actually setting all vars to blank, instead of actually
deleting them.
Leaving issue closed until I hear better suggestions.
Alex

Status changed to Unfortunate.

@davecheney
Copy link
Contributor

Comment 9:

Thanks for the diagnosis Alex.
saar, I'd like to know more about why something (it was mentioned this
was coming from a library, not your code) is calling os.ClearEnv. It
feels to me that there will be a better way to achieve what that code
wants without having to drain the entire environment.
> syscall.Clearenv does not clear process environment. But it has this comment
> "// TODO(bradfitz): pass through to C". So I think it is something that
> still to happen (if anyone cares about it). So I think windows version is
> more correct here. Mind you windows syscall.Clearenv is actually setting all
> vars to blank, instead of actually deleting them.
>
> Leaving issue closed until I hear better suggestions.
Thanks Alex, you are correct. It's hard to describe the behaviour
os.ClearEnv on unix because it is both 1. incorrect, 2. implementation
specific (I have no idea what gccgo does in this case). It's even more
confusing when os/exec is used as the default environment it passes
down to the child *is* the in memory copy, not the original, and that
would be empty if os.ClearEnv is called.

@gopherbot
Copy link
Author

Comment 10 by saar@countersoft.com:

Alex,
From my tests it is only the %SYSTEMROOT% that is causing issues. 
Agreed about closing it, as it is just a gotcha really.
Dave,
Seems like there was a confusion here about os.ClearEnv(), we explicitly called it from
our code. So as above, it is only a gotcha really on Windows 64bit.

@golang golang locked and limited conversation to collaborators Jun 25, 2016
This issue was closed.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

4 participants