I like dark reader too, with the most gpu intensive mode and sepia it makes for some really good dark modes for most websites. And I’ve built dark mode css off of it as an inspiration.
Edit: the "Dynamic" (GPU) mode doesn't work on all sites. On HN I'll use Filter+ with the settings -20 brightness, contrast off, sepia +30, grayscale off.
While on Github I'll do -5, 0, +30, 0. Mainly just tweaking the brightness while having Sepia on most sites.
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.
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!
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.
Hi 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.
Well 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.
> 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.
Hmm 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.
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...
one bad thing about this is that if you also like to redshift your color temperature, you cannot have that and the OS-level invert on at the same time (last time I checked). Hence, inverting the pdf file itself allows you to have the best of both worlds.
the issue we had was that we were using a CSS filter like other commenters mentioned.
The problem is that it inverts images too and pdfjs doesn't actually specify which parts of the document are an image as it's just writing to a canvas.
My plan moving forward is to fix pdfjs so it can invert natively and the actual canvas is inverted so that images won't be inverted.
I think figures would STILL be a problem though.
EPUBs are much easier and we're not inverting there because we can see which one is an img.
37 comments
[ 2.8 ms ] story [ 88.1 ms ] thread> Configure the dark theme: brightness, contrast and sepia. Enable for all websites or particular domains.
Works for PDFs too.
No affiliation.
Edit: the "Dynamic" (GPU) mode doesn't work on all sites. On HN I'll use Filter+ with the settings -20 brightness, contrast off, sepia +30, grayscale off.
While on Github I'll do -5, 0, +30, 0. Mainly just tweaking the brightness while having Sepia on most sites.
chrome://flags/#pdf-viewer-update
Bookmarklet
javascript:(function() { var v = document.getElementsByTagName("html"); v[0].style.background = "white"; v[0].style.filter = "invert(90%) sepia(60%) brightness(70%)"; v[0].style.backgroundColor = "black"; document.getElementsByTagName("body")[0].style.background = "white"; })();
Edit: It appears to be Evince [0]
[0] https://en.wikipedia.org/wiki/Evince
using go to wrap a python script - this is some "hacker man" stuff right here
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:
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.
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:
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/5fe9f505779bb9d...
[1]: https://github.com/rootVIII/pdfinverter/blob/6cbcd4cc7254514...
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.
That said, I've seen messier things than python-in-go shipped and used reliably in production to solve business problems.
https://getpolarized.io/2020/10/05/Polar-2-0-Release.html
the issue we had was that we were using a CSS filter like other commenters mentioned.
The problem is that it inverts images too and pdfjs doesn't actually specify which parts of the document are an image as it's just writing to a canvas.
My plan moving forward is to fix pdfjs so it can invert natively and the actual canvas is inverted so that images won't be inverted.
I think figures would STILL be a problem though.
EPUBs are much easier and we're not inverting there because we can see which one is an img.