4 ms·
And written to be less readable with e.g.: return bytes.ReplaceAll(script, []byte{0x09}, []byte{0x20, 0x20, 0x20, 0x20}) instead of bytes.ReplaceAll(
by 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 agoThanks 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 agoIt 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 agookay have some of these fixes in progress. Thanks again for taking the time to look at my code! Very appreciative
- rootVIII 6y agoHi TheDong. I put most of the changes you recommended and I am much happier with the overall project. I decided to remove the Python part though and went back to the Imagick bindings. There's probably still some things to fix but thanks again for taking the time to look and advise.
- rootVIII 6y agoWell any old working directory that exists at startup is removed. So an attacker would not be able to create the directory before the user runs the program. And they would have to be logged in as the user in order to edit the pngtopdf.py file as it is created with 0700. Also, I would rather specify the actual bytes directly and force a lazy programmer to look at the ASCII table over using []byte(" "). These are just some of my after-thoughts.
- TheDong 6y ago> So an attacker would not be able to create the directory before the user runs the program I said it was a race. An attacker would race with the program's deletion of the directory to recreate it. This race is really easy to do, and there have been numerous CVEs for this sort of race. > And they would have to be logged in as the user in order to edit the pngtopdf.py file as it is created with 0700 That's not true. If I own a directory, I can delete and recreate the files in that directory, even if I don't own the file, even if the file is 0700. Feel free to experiment around to see this.
- rootVIII 6y agoHmm okay. I'm going to start fixing that part and also attempt some CGO/objective-C to replace the python dependency. I had figured that removing any possible directory that could be seen as an application directory (at app-startup) would alleviate the issue you described. Thanks for the feedback though and also taking the time to look.