3 ms·

https://github.com/rootVIII/pdfinverter/blob/master/utils.go#L88 https://github.com/rootVIII/pdfinverter/blob/master/utils.go... using go to wrap a python scri

by xyst 6y ago

https://github.com/rootVIII/pdfinverter/blob/master/utils.go#L88 https://github.com/rootVIII/pdfinverter/blob/master/utils.go... using go to wrap a python script - this is some "hacker man" stuff right here

pepemon 6y ago
But using boring Python only, you are unable to add fancy Go mention to the submission's title!
Hamuko 6y ago
Using Go to wrap a Python script that is mostly just a wrapper for some macOS APIs.
TheDong 6y ago
And written to be less readable with e.g.: return bytes.ReplaceAll(script, []byte{0x09}, []byte{0x20, 0x20, 0x20, 0x20}) instead of bytes.ReplaceAll(script, []byte("\t"), []byte(" ")) Skimming through the code, there's a lot of other pretty basic mistakes. The most egregious is that the use of a python script has a security issue. Let's see if you can spot it, it's in these lines: tmp := fmt.Sprintf("/var/tmp/invertpdf--%s/", time.Now().Format("20060102150405")) os.Mkdir(tmp, 0700) WriteText(tmp+"pngtopdf.py", GetPDFConv()) // later execute that .py file So, what's the issue? Well, using a predictable temporary directory and then not checking for an error in `mkdir` means that an attacker can easily create that directory before you do (especially since it's a predictable name based on the time), and then write their own python script. That lets another user on your machine run arbitrary code as your user. "But", you might say, "WriteText does an os.Exit if it can't write the file". That doesn't matter. If i create the directory with permissions 777, and then have a program waiting for the python script to be written in order to replace it with a malicious script, Mkdir will error (dir already exists), but WriteText will succeed, and so the vulnerability still happens. This is the sort of dumb vulnerability you get if you don't know that `ioutil.TempDir` exists (or don't know about symlink races, tmpdir races, etc). `ioutil.TempDir("/var/tmp", "invertpdf-")` would be the more secure way to do this, though obviously you still should check that error too. There's a lot of other problems with this program, but this vulnerability is the most obvious.
- rootVIII 6y ago
  Thanks for the feedback. Yeah the Python part is definitely a bit hackish. Do you have any other recommendations on other problems? I'm still learning Golang and would like to improve. Thanks!
  - TheDong 6y ago
    It took me a few to respond, but I'll make some recommendations now: 1. Use filepath.Glob if you want to glob Most of [0] can be replaced with filepath.Glob. You should probably also respect TMPDIR if it's set, which ioutil.TempDir etc will do. 2. ExitErr shouldn't be used, especially not within nested functions. You have 'defer os.RemoveAll(tmpDir)'. defers won't get called if you `os.Exit(1)` as you do in ExitErr. You should be instead doing `return nil, err` all the way back up to `err := RunApp()`, and printing the error there. That will let defers run. In general, using os.Exit anywhere other than in the main function is an antipattern. 3. Your parallel processing should be using a waitgroup or errgroup. A slice of channels is super fragile. In [1], do something like the following: // before the for loop var wg sync.WaitGroup // in each iteration of the for loop wg.Add(1) go func(name string) { defer wg.Done() cli.ImageRoutine(fileName) }(fileName) // at the end, to wait wg.Wait() Use an errgroup if you need to cancel it. The channel thing you're doing now is more fragile than a waitgroup, and harder to reason about. 4. use of exported functions / unexported functions is all over the place and inconsistent. There's a few other things, but that's what I've got offhand. I can't actually run the code because I only use linux, and it obv doesn't run on linux. Oh, I guess that's another thing, you want some build tags to make it more clear it won't run on linux. Hopefully something in there was helpful [0]: https://github.com/rootVIII/pdfinverter/blob/5fe9f505779bb9d75b8b3edc879c18065bfd0a3c/utils.go#L53-L62 https://github.com/rootVIII/pdfinverter/blob/5fe9f505779bb9d... [1]: https://github.com/rootVIII/pdfinverter/blob/6cbcd4cc72545142247c859073fd02e952e73b10/cli.go#L19-L20 https://github.com/rootVIII/pdfinverter/blob/6cbcd4cc7254514...
    - rootVIII 6y ago
      okay have some of these fixes in progress. Thanks again for taking the time to look at my code! Very appreciative
shoo 6y ago
It may be possible to replace the dependency on the python script by using objective c to call the required libraries, then expose a c API that can be called from the go application. E.g. https://gist.github.com/porty/6b94f7f908ed7af0f070 https://gist.github.com/porty/6b94f7f908ed7af0f070 https://github.com/caseymrm/menuet https://github.com/caseymrm/menuet That said, I've seen messier things than python-in-go shipped and used reliably in production to solve business problems.
- jamil7 6y ago
  That or just write the whole script in Python and avoid a needless Go dependency.
- rootVIII 6y ago
  That's a fantastic idea. I actually looked into it because I really didn't want to use Python to create the output PDFs. I have experience making C bindings in Go, but had issues with Objective C (I also don't know Obj-C). If you are able to remove the Python dependency with a CGo please open a PR! Thanks for the feedback too...

by xyst 6y ago

pepemon 6y ago

But using boring Python only, you are unable to add fancy Go mention to the submission's title!

Hamuko 6y ago

Using Go to wrap a Python script that is mostly just a wrapper for some macOS APIs.

TheDong 6y ago

And written to be less readable with e.g.: return bytes.ReplaceAll(script, []byte{0x09}, []byte{0x20, 0x20, 0x20, 0x20}) instead of bytes.ReplaceAll(script, []byte("\t"), []byte(" ")) Skimming through the code, there's a lot of other pretty basic mistakes. The most egregious is that the use of a python script has a security issue. Let's see if you can spot it, it's in these lines: tmp := fmt.Sprintf("/var/tmp/invertpdf--%s/", time.Now().Format("20060102150405")) os.Mkdir(tmp, 0700) WriteText(tmp+"pngtopdf.py", GetPDFConv()) // later execute that .py file So, what's the issue? Well, using a predictable temporary directory and then not checking for an error in `mkdir` means that an attacker can easily create that directory before you do (especially since it's a predictable name based on the time), and then write their own python script. That lets another user on your machine run arbitrary code as your user. "But", you might say, "WriteText does an os.Exit if it can't write the file". That doesn't matter. If i create the directory with permissions 777, and then have a program waiting for the python script to be written in order to replace it with a malicious script, Mkdir will error (dir already exists), but WriteText will succeed, and so the vulnerability still happens. This is the sort of dumb vulnerability you get if you don't know that `ioutil.TempDir` exists (or don't know about symlink races, tmpdir races, etc). `ioutil.TempDir("/var/tmp", "invertpdf-")` would be the more secure way to do this, though obviously you still should check that error too. There's a lot of other problems with this program, but this vulnerability is the most obvious.

rootVIII 6y ago
Thanks for the feedback. Yeah the Python part is definitely a bit hackish. Do you have any other recommendations on other problems? I'm still learning Golang and would like to improve. Thanks!
- TheDong 6y ago
  It took me a few to respond, but I'll make some recommendations now: 1. Use filepath.Glob if you want to glob Most of [0] can be replaced with filepath.Glob. You should probably also respect TMPDIR if it's set, which ioutil.TempDir etc will do. 2. ExitErr shouldn't be used, especially not within nested functions. You have 'defer os.RemoveAll(tmpDir)'. defers won't get called if you `os.Exit(1)` as you do in ExitErr. You should be instead doing `return nil, err` all the way back up to `err := RunApp()`, and printing the error there. That will let defers run. In general, using os.Exit anywhere other than in the main function is an antipattern. 3. Your parallel processing should be using a waitgroup or errgroup. A slice of channels is super fragile. In [1], do something like the following: // before the for loop var wg sync.WaitGroup // in each iteration of the for loop wg.Add(1) go func(name string) { defer wg.Done() cli.ImageRoutine(fileName) }(fileName) // at the end, to wait wg.Wait() Use an errgroup if you need to cancel it. The channel thing you're doing now is more fragile than a waitgroup, and harder to reason about. 4. use of exported functions / unexported functions is all over the place and inconsistent. There's a few other things, but that's what I've got offhand. I can't actually run the code because I only use linux, and it obv doesn't run on linux. Oh, I guess that's another thing, you want some build tags to make it more clear it won't run on linux. Hopefully something in there was helpful [0]: https://github.com/rootVIII/pdfinverter/blob/5fe9f505779bb9d75b8b3edc879c18065bfd0a3c/utils.go#L53-L62 https://github.com/rootVIII/pdfinverter/blob/5fe9f505779bb9d... [1]: https://github.com/rootVIII/pdfinverter/blob/6cbcd4cc72545142247c859073fd02e952e73b10/cli.go#L19-L20 https://github.com/rootVIII/pdfinverter/blob/6cbcd4cc7254514...
  - rootVIII 6y ago
    okay have some of these fixes in progress. Thanks again for taking the time to look at my code! Very appreciative

shoo 6y ago

It may be possible to replace the dependency on the python script by using objective c to call the required libraries, then expose a c API that can be called from the go application. E.g. https://gist.github.com/porty/6b94f7f908ed7af0f070 https://gist.github.com/porty/6b94f7f908ed7af0f070 https://github.com/caseymrm/menuet https://github.com/caseymrm/menuet That said, I've seen messier things than python-in-go shipped and used reliably in production to solve business problems.

jamil7 6y ago
That or just write the whole script in Python and avoid a needless Go dependency.
rootVIII 6y ago
That's a fantastic idea. I actually looked into it because I really didn't want to use Python to create the output PDFs. I have experience making C bindings in Go, but had issues with Objective C (I also don't know Obj-C). If you are able to remove the Python dependency with a CGo please open a PR! Thanks for the feedback too...