traversc / qs

Quick serialization of R objects
397 stars 19 forks source link

Deserialization arbitrary code execution attack #93

Open thomascwells opened 4 months ago

thomascwells commented 4 months ago

Is the qs format susceptible to the same type of deserialization attack underlying the recent RDS CVE?

References:

traversc commented 4 months ago

Thank you, I will look into it.

TimTaylor commented 4 months ago

FWIW - AFAICT yes. You can use qs to replicate the following example given by George Stagg in https://mstdn.social/@gws/112359739655466497

traversc commented 4 months ago

Updated on github. R 4.4 throws an error if you try to unserialize a promise. I actually think that's the wrong approach, instead I replace it with NULL and issue a warning.

e <- new.env()
delayedAssign("x", system("uname -msrn"), assign.env = e)
qsave(e, "/mnt/n/temp.qs")
qread("/mnt/n/temp.qs")$x
# Warning message:
#   In qread("/mnt/n/temp.qs") :
#   PROMSXP detected, replacing with NULL (see https://github.com/traversc/qs/issues/93)

Editorial: I agree with gws on mastadon, I don't think this is a big deal, the issue is equivalent to if you ran source on an unknown file. Make sure you know where your files come from!

TimTaylor commented 4 months ago

Editorial: I agree with gws on mastadon, I don't think this is a big deal, the issue is equivalent to if you ran source on an unknown file. Make sure you know where your files come from!

Agree completely. Mainly came to flag for awareness and saw the issue already.

traversc commented 3 months ago

On CRAN now

sda030 commented 3 months ago

Hm, I understand the security concern, but the patch is breaking a few hundred ggplot2/ggiraph objects stored to disk being used in our reports (longer story why we pre-store these to disk). Except for locally rolling back to qs 0.26.1 (0.26.2 is not in the CRAN archive?), is there a way to avoid replacing the PROMSXP with NULL if I trust these files? An override_safety argument perhaps? The gg objects would also be somewhat cumbersome to reproduce and store as new file (formats) now in our production process.

traversc commented 3 months ago

An override argument sounds reasonable.

traversc commented 3 months ago

@sda030 Try the latest commit with your previous ggplots and let me know if it works.

New function set_trust_promises

Standard behavior: evaluate promises during save, don't allow promises during read.

e <- new.env()
delayedAssign("x", system("uname -msrn"), assign.env = e)

qsave(e, "/mnt/n/temp.qs")
# Linux Travers-PC 5.15.146.1-microsoft-standard-WSL2 x86_64

qread("/mnt/n/temp.qs")$x
[1] 0

With set_trust_promises

e <- new.env()
delayedAssign("x", system("uname -msrn"), assign.env = e)

set_trust_promises(TRUE)
qsave(e, "/mnt/n/temp.qs")

qread("/mnt/n/temp.qs")$x
# Linux Travers-PC 5.15.146.1-microsoft-standard-WSL2 x86_64
# [1] 0
sda030 commented 3 months ago

Thanks for quick response @traversc! And sorry for my late response. It works for your example. Tough a bit weird that when setting set_trust_promises(TRUE), I get a FALSE in return? Should not the returned value be the value being changed to, not the value it was? (Alternatively consider a more informative "Previous value: FALSE") Very much like the idea of a global option. However, this setting only applies before saving files, which is a bit late for my already saved plots..?

P.S. Will completely change my setup this summer to avoid pre-storing the plots for the future.

traversc commented 3 months ago

The global option changes both saving and reading files, so you should be able to read previously saved promises from earlier versions of R / qs.

sda030 commented 3 months ago

I swear it did not work the first time, but maybe I missed a step. Now it works consistently, also with reading in a ggobj. I also tested turning it off and on again. Nice! Thank you very much for swift action. :)

sda030 commented 2 months ago

@traversc, may I politely request a patch release on CRAN before August? (I try to protect my less-technically oriented colleagues from R's many ideosyncracies such as Rtools, devtools::install_github(), etc)