Bug#1021842: Finalizing 'inhibit-automatic-native-compilation'
Tatsuya Kinoshita writes: > On 2023-02-20 at 20:22 +, Andrea Corallo wrote: >> I've installed 5d0b45cd67b on emacs-29 in order to use always >> `make-temp-file'. > > Please be careful with the difference between make-temp-file-internal > and make-temp-file. > >> +++ b/lisp/emacs-lisp/comp.el >> (expand-file-name >> - (make-temp-file-internal (file-name-sans-extension >> rel-filename) >> - 0 ".eln" nil) >> + (make-temp-file (file-name-sans-extension rel-filename) 0 >> ".eln" >> + nil) >> temporary-file-directory > > This second argument of make-temp-file should be nil, and expanding > against temporary-file-directory is already done by make-temp-file. > > Thanks, Hi Tatsuya, thanks for checking, 68df9e5953c fix that. Bests Andrea
Bug#1021842: Finalizing 'inhibit-automatic-native-compilation'
On Mon, Feb 20, 2023, 4:34 PM Stefan Monnier wrote: > > Just to be clear, this condition should be checked before emacs is > > willing to use the temporary directory in question. No unprivileged > > user should be able to overwrite a directory entry the uid of the > > emacs process creates at any point in the path to the temporary file. > > AFAIK we usually consider it's up to the user/system to make sure this > is the case. That seems like a pretty aggressive assumption for this functionality. Lynn
Bug#1021842: Finalizing 'inhibit-automatic-native-compilation'
On 2023-02-20 at 20:22 +, Andrea Corallo wrote: > I've installed 5d0b45cd67b on emacs-29 in order to use always > `make-temp-file'. Please be careful with the difference between make-temp-file-internal and make-temp-file. > +++ b/lisp/emacs-lisp/comp.el > (expand-file-name > - (make-temp-file-internal (file-name-sans-extension rel-filename) > - 0 ".eln" nil) > + (make-temp-file (file-name-sans-extension rel-filename) 0 ".eln" > + nil) > temporary-file-directory This second argument of make-temp-file should be nil, and expanding against temporary-file-directory is already done by make-temp-file. Thanks, -- Tatsuya Kinoshita
Bug#1021842: Finalizing 'inhibit-automatic-native-compilation'
> Just to be clear, this condition should be checked before emacs is > willing to use the temporary directory in question. No unprivileged > user should be able to overwrite a directory entry the uid of the > emacs process creates at any point in the path to the temporary file. AFAIK we usually consider it's up to the user/system to make sure this is the case. Stefan
Bug#1021842: Finalizing 'inhibit-automatic-native-compilation'
On Mon, Feb 20, 2023 at 11:02 AM Stefan Monnier wrote: > > So I guess one could remove the file after the first creation and make > > it a link pointing to some other file waiting for libgccjit to do > > its write. > > "One" as in "an attacker"? In `/tmp` an attacker should not be able to > do that because it's supposed to be using the sticky bit so that only > the owner of a file can remove it. Just to be clear, this condition should be checked before emacs is willing to use the temporary directory in question. No unprivileged user should be able to overwrite a directory entry the uid of the emacs process creates at any point in the path to the temporary file. Lynn
Bug#1021842: Finalizing 'inhibit-automatic-native-compilation'
Eli Zaretskii writes: >> From: Andrea Corallo >> Cc: monn...@iro.umontreal.ca, t...@debian.org, emacs-de...@gnu.org, >> spwhit...@spwhitton.name, 1021...@bugs.debian.org >> Date: Mon, 20 Feb 2023 15:42:08 + >> >> > You mean, from master to emacs-29, I guess? >> >> Yes >> >> > What was the motivation for that change? The log message doesn't >> > explain the problem it meant to solve in enough detail, it just says >> > something about creating the file twice? How can that happen? who >> > creates the file the second time? >> >> Before e6043641d30 the file was created by Fmake_temp_file_internal and >> afterwards overwritten by libgccjit. So I guess one could remove the >> file after the first creation and make it a link pointing to some other >> file waiting for libgccjit to do its write. > > Then it's okay to cherry-pick it to emacs-29. (I actually am not sure > why we didn't install it on emacs-29 to begin with, but never mind.) I didn't install it at the time in emacs-29 as I thought this had nothing to do with security. Anyway this turned out not to be the best solution, so after today's discussion in this thread I've installed 5d0b45cd67b into emacs-29 as should be the optimal fix. Note there will be conflict in mergin 29 into 30 and we'll have to accept the change in 29 (sorry never managed to get gitmerge.el working here). Best Regards Andrea
Bug#1021842: Finalizing 'inhibit-automatic-native-compilation'
Stefan Monnier writes: >> Before e6043641d30 the file was created by Fmake_temp_file_internal and >> afterwards overwritten by libgccjit. > > Yes, that was good. > >> So I guess one could remove the file after the first creation and make >> it a link pointing to some other file waiting for libgccjit to do >> its write. > > "One" as in "an attacker"? In `/tmp` an attacker should not be able to > do that because it's supposed to be using the sticky bit so that only > the owner of a file can remove it. Finally understood thanks! I've installed 5d0b45cd67b on emacs-29 in order to use always `make-temp-file'. I think at the time I preferred Fmake_temp_file_internal not to call into Lisp for some reason I can't remember now, anyway seems to work now for me (just bootstrapped successfully). Andrea
Bug#1021842: Finalizing 'inhibit-automatic-native-compilation'
> From: Andrea Corallo > Cc: monn...@iro.umontreal.ca, t...@debian.org, emacs-de...@gnu.org, > spwhit...@spwhitton.name, 1021...@bugs.debian.org > Date: Mon, 20 Feb 2023 15:42:08 + > > > You mean, from master to emacs-29, I guess? > > Yes > > > What was the motivation for that change? The log message doesn't > > explain the problem it meant to solve in enough detail, it just says > > something about creating the file twice? How can that happen? who > > creates the file the second time? > > Before e6043641d30 the file was created by Fmake_temp_file_internal and > afterwards overwritten by libgccjit. So I guess one could remove the > file after the first creation and make it a link pointing to some other > file waiting for libgccjit to do its write. Then it's okay to cherry-pick it to emacs-29. (I actually am not sure why we didn't install it on emacs-29 to begin with, but never mind.) Thanks.
Bug#1021842: Finalizing 'inhibit-automatic-native-compilation'
Stefan Monnier writes: >>> `make-temp-name` uses `O_EXCL | O_CREAT` so as to close the race >>> condition: if someone predicated the filename, we detect it atomically >>> and we try again. >>> >>> You might like to check >>> >>> >>> https://wiki.sei.cmu.edu/confluence/display/c/FIO21-C.+Do+not+create+temporary+files+in+shared+directories >> >> Thanks for the pointer. >> >> I'm still not really convinced we have a problem here with trampolines. >> With `make-temp-file' we are really only choosing the filename and >> suggesting it to libgccjit, this last one will perform the file >> creation. > > The important part is the use of `O_EXCL | O_CREAT` when creating > the file. > *BUT* `O_EXCL | O_CREAT` will fail if the file already exists. Which is > why `make-temp-file` needs `make-temp-name` to generate new names until > we find one that really doesn't exist (not just at the time > `make-temp-name` is called but the fraction of a millisecond later when > we do try to create it). We can't use this loop, we tipically pass a filename to be used to libgccjit and we have no control after (also see my last comment). >> I'd be surprised if GCC does not handle this correctly, and >> in case shouldn't this be a GCC bug? > > I'd be surprised. Surprised if it does or does not? > If you tell it to write to a pre-existing file, does > it fail with an error? I believe it does not. > If not, then I think it can't be used safely unless > *you* pre-create the file (e.g. with `make-temp-file`). Are we sure? Also if I pre-create the file with make-temp-file can't someone just replace it even more easily with the infamous link before libgccjit comes in? Thanks Andrea
Bug#1021842: Finalizing 'inhibit-automatic-native-compilation'
> Before e6043641d30 the file was created by Fmake_temp_file_internal and > afterwards overwritten by libgccjit. Yes, that was good. > So I guess one could remove the file after the first creation and make > it a link pointing to some other file waiting for libgccjit to do > its write. "One" as in "an attacker"? In `/tmp` an attacker should not be able to do that because it's supposed to be using the sticky bit so that only the owner of a file can remove it. Stefan
Bug#1021842: Finalizing 'inhibit-automatic-native-compilation'
Eli Zaretskii writes: >> From: Andrea Corallo >> Cc: Tatsuya Kinoshita , emacs-de...@gnu.org, >> spwhit...@spwhitton.name, 1021...@bugs.debian.org, Eli Zaretskii >> >> Date: Mon, 20 Feb 2023 09:03:34 + >> >> OTOH on a slightly differnt subject and in light of this, I think we >> should probably backport e6043641d30 into emacs-30, so that eln files >> are created onace and only by libgccjit. Eli WDYT? > > You mean, from master to emacs-29, I guess? Yes > What was the motivation for that change? The log message doesn't > explain the problem it meant to solve in enough detail, it just says > something about creating the file twice? How can that happen? who > creates the file the second time? Before e6043641d30 the file was created by Fmake_temp_file_internal and afterwards overwritten by libgccjit. So I guess one could remove the file after the first creation and make it a link pointing to some other file waiting for libgccjit to do its write. Andrea
Bug#1021842: Finalizing 'inhibit-automatic-native-compilation'
>> `make-temp-name` uses `O_EXCL | O_CREAT` so as to close the race >> condition: if someone predicated the filename, we detect it atomically >> and we try again. >> >> You might like to check >> >> >> https://wiki.sei.cmu.edu/confluence/display/c/FIO21-C.+Do+not+create+temporary+files+in+shared+directories > > Thanks for the pointer. > > I'm still not really convinced we have a problem here with trampolines. > With `make-temp-file' we are really only choosing the filename and > suggesting it to libgccjit, this last one will perform the file > creation. The important part is the use of `O_EXCL | O_CREAT` when creating the file. *BUT* `O_EXCL | O_CREAT` will fail if the file already exists. Which is why `make-temp-file` needs `make-temp-name` to generate new names until we find one that really doesn't exist (not just at the time `make-temp-name` is called but the fraction of a millisecond later when we do try to create it). > I'd be surprised if GCC does not handle this correctly, and > in case shouldn't this be a GCC bug? I'd be surprised. If you tell it to write to a pre-existing file, does it fail with an error? If not, then I think it can't be used safely unless *you* pre-create the file (e.g. with `make-temp-file`). Stefan
Bug#1021842: Finalizing 'inhibit-automatic-native-compilation'
> From: Andrea Corallo > Cc: Tatsuya Kinoshita , emacs-de...@gnu.org, > spwhit...@spwhitton.name, 1021...@bugs.debian.org, Eli Zaretskii > > Date: Mon, 20 Feb 2023 09:18:02 + > > Andrea Corallo writes: > > > Stefan Monnier writes: > > > >>> Shouldn't make-temp-file-internal return a non predictable file name? > >> > >> Nope. It's less predictable but it's still predictable. > >> > >>> Otherwise what's the point of using make-temp-file in the first place if > >>> the temporary name is predictable? > >> > >> `make-temp-name` uses `O_EXCL | O_CREAT` so as to close the race > >> condition: if someone predicated the filename, we detect it atomically > >> and we try again. > >> > >> You might like to check > >> > >> > >> https://wiki.sei.cmu.edu/confluence/display/c/FIO21-C.+Do+not+create+temporary+files+in+shared+directories > > > > Thanks for the pointer. > > > > I'm still not really convinced we have a problem here with trampolines. > > With `make-temp-file' we are really only choosing the filename and > > suggesting it to libgccjit, this last one will perform the file > > creation. I'd be surprised if GCC does not handle this correctly, and > > in case shouldn't this be a GCC bug? > > > > OTOH on a slightly differnt subject and in light of this, I think we > > should probably backport e6043641d30 into emacs-30, so that eln files > > are created onace and only by libgccjit. Eli WDYT? > > And BTW, probably also in emacs-28 if we are doing another release. We don't want to make any enhancements in Emacs 28.3, just to plug the security hole. People who want enhancements will need to wait for Emacs 29.1, hopefully not too long.
Bug#1021842: Finalizing 'inhibit-automatic-native-compilation'
> From: Andrea Corallo > Cc: Tatsuya Kinoshita , emacs-de...@gnu.org, > spwhit...@spwhitton.name, 1021...@bugs.debian.org, Eli Zaretskii > > Date: Mon, 20 Feb 2023 09:03:34 + > > OTOH on a slightly differnt subject and in light of this, I think we > should probably backport e6043641d30 into emacs-30, so that eln files > are created onace and only by libgccjit. Eli WDYT? You mean, from master to emacs-29, I guess? What was the motivation for that change? The log message doesn't explain the problem it meant to solve in enough detail, it just says something about creating the file twice? How can that happen? who creates the file the second time?
Bug#1021842: Finalizing 'inhibit-automatic-native-compilation'
Andrea Corallo writes: > Stefan Monnier writes: > >>> Shouldn't make-temp-file-internal return a non predictable file name? >> >> Nope. It's less predictable but it's still predictable. >> >>> Otherwise what's the point of using make-temp-file in the first place if >>> the temporary name is predictable? >> >> `make-temp-name` uses `O_EXCL | O_CREAT` so as to close the race >> condition: if someone predicated the filename, we detect it atomically >> and we try again. >> >> You might like to check >> >> >> https://wiki.sei.cmu.edu/confluence/display/c/FIO21-C.+Do+not+create+temporary+files+in+shared+directories > > Thanks for the pointer. > > I'm still not really convinced we have a problem here with trampolines. > With `make-temp-file' we are really only choosing the filename and > suggesting it to libgccjit, this last one will perform the file > creation. I'd be surprised if GCC does not handle this correctly, and > in case shouldn't this be a GCC bug? > > OTOH on a slightly differnt subject and in light of this, I think we > should probably backport e6043641d30 into emacs-30, so that eln files > are created onace and only by libgccjit. Eli WDYT? And BTW, probably also in emacs-28 if we are doing another release. Andrea
Bug#1021842: Finalizing 'inhibit-automatic-native-compilation'
Stefan Monnier writes: >> Shouldn't make-temp-file-internal return a non predictable file name? > > Nope. It's less predictable but it's still predictable. > >> Otherwise what's the point of using make-temp-file in the first place if >> the temporary name is predictable? > > `make-temp-name` uses `O_EXCL | O_CREAT` so as to close the race > condition: if someone predicated the filename, we detect it atomically > and we try again. > > You might like to check > > > https://wiki.sei.cmu.edu/confluence/display/c/FIO21-C.+Do+not+create+temporary+files+in+shared+directories Thanks for the pointer. I'm still not really convinced we have a problem here with trampolines. With `make-temp-file' we are really only choosing the filename and suggesting it to libgccjit, this last one will perform the file creation. I'd be surprised if GCC does not handle this correctly, and in case shouldn't this be a GCC bug? OTOH on a slightly differnt subject and in light of this, I think we should probably backport e6043641d30 into emacs-30, so that eln files are created onace and only by libgccjit. Eli WDYT? Thanks Andrea
Bug#1021842: Finalizing 'inhibit-automatic-native-compilation'
On 2023-02-18 at 21:56 +, Andrea Corallo wrote: > >> +(expand-file-name > >> + (make-temp-file-internal (file-name-sans-extension > >> rel-filename) > >> + 0 ".eln" nil) > >> + temporary-file-directory > > > > Hmm, it seems using make-temp-file-internal with DIR-FLAG=0 which just > > constructs a name and do not create the file like make-temp-name, so > > there is a race condition as Stefan mentioned. Is that really OK? > > Mmhh, Stefan mentioned the case where the tmp file name is predicted. > > Shouldn't make-temp-file-internal return a non predictable file name? > Otherwise what's the point of using make-temp-file in the first place if > the temporary name is predictable? Imagine if a local attacker creates symlinks as the candidate names before creating the file, though less predictable. make-temp-name describes as follows: > There is a race condition between calling `make-temp-name' and > later creating the file, which opens all kinds of security holes. > For that reason, you should normally use `make-temp-file' instead. To create a temporary file in a secure fashion, use make-temp-file to create a file, or use make-temp-file with DIR-FLAG to create a subdirectory and then create a file in it. Thanks, -- Tatsuya Kinoshita
Bug#1021842: Finalizing 'inhibit-automatic-native-compilation'
> Shouldn't make-temp-file-internal return a non predictable file name? Nope. It's less predictable but it's still predictable. > Otherwise what's the point of using make-temp-file in the first place if > the temporary name is predictable? `make-temp-name` uses `O_EXCL | O_CREAT` so as to close the race condition: if someone predicated the filename, we detect it atomically and we try again. You might like to check https://wiki.sei.cmu.edu/confluence/display/c/FIO21-C.+Do+not+create+temporary+files+in+shared+directories -- Stefan
Bug#1021842: Finalizing 'inhibit-automatic-native-compilation'
Tatsuya Kinoshita writes: > On 2023-02-17 at 09:42 -0700, Sean Whitton wrote: >> So: commit ce4a066ed1e fixes Debian bug #1021842 without the env var. > > On 2023-02-14 at 11:32 +, Andrea Corallo wrote: >> Stefan Monnier writes: >> > `temporary-file-directory' may point to a world-writable directory, so >> > it's vulnerable to the usual race condition where someone manages to >> > predict the name of the file you're going to write and places there >> > a symlink to some "interesting" place, so you end up overwriting some >> > other file unwittingly. >> >> Okay, ce4a066ed1e generates trampolines in a temporary directory if no >> other option is viable (using the make-temp-file machinery to generate >> the unpredictable name). > >> + finally (cl-return >> +(expand-file-name >> + (make-temp-file-internal (file-name-sans-extension >> rel-filename) >> + 0 ".eln" nil) >> + temporary-file-directory > > Hmm, it seems using make-temp-file-internal with DIR-FLAG=0 which just > constructs a name and do not create the file like make-temp-name, so > there is a race condition as Stefan mentioned. Is that really OK? Mmhh, Stefan mentioned the case where the tmp file name is predicted. Shouldn't make-temp-file-internal return a non predictable file name? Otherwise what's the point of using make-temp-file in the first place if the temporary name is predictable? Andrea
Bug#1021842: Finalizing 'inhibit-automatic-native-compilation'
On 2023-02-17 at 09:42 -0700, Sean Whitton wrote: > So: commit ce4a066ed1e fixes Debian bug #1021842 without the env var. On 2023-02-14 at 11:32 +, Andrea Corallo wrote: > Stefan Monnier writes: > > `temporary-file-directory' may point to a world-writable directory, so > > it's vulnerable to the usual race condition where someone manages to > > predict the name of the file you're going to write and places there > > a symlink to some "interesting" place, so you end up overwriting some > > other file unwittingly. > > Okay, ce4a066ed1e generates trampolines in a temporary directory if no > other option is viable (using the make-temp-file machinery to generate > the unpredictable name). > + finally (cl-return > +(expand-file-name > + (make-temp-file-internal (file-name-sans-extension rel-filename) > + 0 ".eln" nil) > + temporary-file-directory Hmm, it seems using make-temp-file-internal with DIR-FLAG=0 which just constructs a name and do not create the file like make-temp-name, so there is a race condition as Stefan mentioned. Is that really OK? Thanks, -- Tatsuya Kinoshita
Bug#1021842: Finalizing 'inhibit-automatic-native-compilation'
> From: Sean Whitton > Cc: Eli Zaretskii , aymeric.a...@yandex.com, > monn...@iro.umontreal.ca, emacs-de...@gnu.org, la...@gnus.org, > r...@defaultvalue.org > Date: Fri, 17 Feb 2023 09:42:29 -0700 > > > Otherwise you have to revert probably 654428b65ae and 2f28496d038 and > > cherry pick in order 1795839babc 5d0912f1445 and ce4a066ed1e from > > feature/inhibit-native-comp-cleanup. But this is a blind guess, I can't > > guarantee it to work without trying it. > > Those are the right ones! > > So: commit ce4a066ed1e fixes Debian bug #1021842 without the env var. Thanks for testing.
Bug#1021842: Finalizing 'inhibit-automatic-native-compilation'
Hello, On Fri 17 Feb 2023 at 09:00AM GMT, Andrea Corallo wrote: > can't you just test feature/inhibit-native-comp-cleanup directy? That > would be the safest and simpler option IMO. Not really -- Debian filters out a lot of the source tree due to disagreements with the FSF regarding software freedom, and we have our other patches. So trying it directly it wouldn't give much confidence regarding the original bug filing. > Otherwise you have to revert probably 654428b65ae and 2f28496d038 and > cherry pick in order 1795839babc 5d0912f1445 and ce4a066ed1e from > feature/inhibit-native-comp-cleanup. But this is a blind guess, I can't > guarantee it to work without trying it. Those are the right ones! So: commit ce4a066ed1e fixes Debian bug #1021842 without the env var. Thanks. -- Sean Whitton signature.asc Description: PGP signature
