New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Move compilation daemon portfile to homedir #6108

Merged
merged 2 commits into from Oct 9, 2017

Conversation

Projects
None yet
5 participants
@adriaanm
Member

adriaanm commented Oct 3, 2017

Store the compilation daemon's administrativia (port file, redirection)
under ~/.scalac/, instead of the less standard
/tmp/scala-devel/${USER:shared}/scalac-compile-server-port.

On creation, remove group- and other-permissions from these
private files, ditto for the repl's history file.

On Java 6 on Windows, opt in to compilation daemon using -nc:false.

@adriaanm adriaanm requested a review from retronym Oct 3, 2017

Move compilation daemon portfile under `~/.scalac/`
Store the compilation daemon's administrativia (port file, redirection)
under `~/.scalac/`, instead of the less standard
`/tmp/scala-devel/${USER:shared}/scalac-compile-server-port`.

On creation, remove group- and other-permissions from these
private files, ditto for the repl's history file.

On Java 6 on Windows, opt in to compilation daemon using `-nc:false`.
Show outdated Hide outdated src/reflect/scala/reflect/internal/util/OwnerOnlyChmod.scala Outdated
Show outdated Hide outdated src/compiler/scala/tools/nsc/GenericRunnerSettings.scala Outdated
Show outdated Hide outdated src/compiler/scala/tools/nsc/CompileSocket.scala Outdated
Show outdated Hide outdated src/reflect/scala/reflect/internal/util/OwnerOnlyChmod.scala Outdated
Show outdated Hide outdated src/reflect/scala/reflect/internal/util/OwnerOnlyChmod.scala Outdated
Show outdated Hide outdated src/repl-jline/scala/tools/nsc/interpreter/jline/FileBackedHistory.scala Outdated
try OwnerOnlyChmod().chmodOrCreateEmpty(p.jfile)
catch { case NonFatal(e) =>
if (interpreter.isReplDebug) e.printStackTrace()
interpreter.replinfo(s"Warning: history file ${p}'s permissions could not be restricted to owner-only.")

This comment has been minimized.

@SethTisue

SethTisue Oct 5, 2017

Member

is warning and continuing considered sufficiently secure?

@SethTisue

SethTisue Oct 5, 2017

Member

is warning and continuing considered sufficiently secure?

@adriaanm

This comment has been minimized.

Show comment
Hide comment
@adriaanm

adriaanm Oct 6, 2017

Member

@retronym, ready for final review

Member

adriaanm commented Oct 6, 2017

@retronym, ready for final review

@adriaanm adriaanm added this to the 2.11.12 milestone Oct 6, 2017

@adriaanm

This comment has been minimized.

Show comment
Hide comment
@adriaanm

adriaanm Oct 6, 2017

Member

@szeiger could you give this a go on your surface and any other windows desktops at your disposal ?

Member

adriaanm commented Oct 6, 2017

@szeiger could you give this a go on your surface and any other windows desktops at your disposal ?

@adriaanm adriaanm merged commit f3419fc into scala:2.11.x Oct 9, 2017

6 checks passed

cla @adriaanm signed the Scala CLA. Thanks!
Details
combined All previous commits successful.
integrate-ide [2838] SUCCESS. Took 3 s.
Details
validate-main [3153] SUCCESS. Took 65 min.
Details
validate-publish-core [3256] SUCCESS. Took 6 min.
Details
validate-test [2306] SUCCESS. Took 59 min.
Details
} finally {
fos2.close()
}
}

This comment has been minimized.

@setharnold

setharnold Nov 15, 2017

Note that creating a file then changing the permissions on the file allows a race condition where an attacker on the system can open the file between the file create and the permissions change. It is far safer to use open(2) to set the permissions restrictive while creating the file.

Furthermore if this code is ever run on a directory with shared writing permissions, it's possible for an attacker to unlink(file), set a symlink in place, and have the process change modes of any target that the process has privileges to modify. fchmod(2) is the safe way to change permissions on a file that is already open, though that won't fix the above race condition. So consider this just an aside.

Thanks

@setharnold

setharnold Nov 15, 2017

Note that creating a file then changing the permissions on the file allows a race condition where an attacker on the system can open the file between the file create and the permissions change. It is far safer to use open(2) to set the permissions restrictive while creating the file.

Furthermore if this code is ever run on a directory with shared writing permissions, it's possible for an attacker to unlink(file), set a symlink in place, and have the process change modes of any target that the process has privileges to modify. fchmod(2) is the safe way to change permissions on a file that is already open, though that won't fix the above race condition. So consider this just an aside.

Thanks

This comment has been minimized.

@adriaanm

adriaanm Nov 15, 2017

Member

Thank you. Could you suggest how to do this on Java 6? I agree the race condition is a security hole, but couldn't figure out how to do this atomically with the API we have available. I think our NIO-based implementation on Java 8 is closer to what you're suggesting: https://github.com/scala/scala/pull/6120/files#diff-3578ac76088d22b5d9912e984dee3affR46.

@adriaanm

adriaanm Nov 15, 2017

Member

Thank you. Could you suggest how to do this on Java 6? I agree the race condition is a security hole, but couldn't figure out how to do this atomically with the API we have available. I think our NIO-based implementation on Java 8 is closer to what you're suggesting: https://github.com/scala/scala/pull/6120/files#diff-3578ac76088d22b5d9912e984dee3affR46.

This comment has been minimized.

@retronym

retronym Nov 15, 2017

Member

Thank you for the review.

We implemented things that way (or at least, tried to) in the 2.12 branch where we could link against the Java 8 NIO API. Scala 2.11 still supports Java 6.

https://github.com/scala/scala/blob/2.12.x/src/reflect/scala/reflect/internal/util/OwnerOnlyChmod.scala

@retronym

retronym Nov 15, 2017

Member

Thank you for the review.

We implemented things that way (or at least, tried to) in the 2.12 branch where we could link against the Java 8 NIO API. Scala 2.11 still supports Java 6.

https://github.com/scala/scala/blob/2.12.x/src/reflect/scala/reflect/internal/util/OwnerOnlyChmod.scala

This comment has been minimized.

@retronym

retronym Nov 15, 2017

Member

adriaanm a minute ago
retronym 29 seconds ago

Speaking of race conditions :)

@retronym

retronym Nov 15, 2017

Member

adriaanm a minute ago
retronym 29 seconds ago

Speaking of race conditions :)

This comment has been minimized.

@setharnold

setharnold Nov 17, 2017

The #diff-3578ac7.. link above looks like it may have the same race condition.

I don't know Java particularly well any longer, so it's hard to suggest a replacement. Two possibilities come to mind:

  • set the umask to 0077 (octal), create the file, set the modes, and then reset umask to whatever the user wanted
  • chmod 0700 (octal) the containing directory, create the file, set the file modes, then reset the directory to whatever permissions it should have.

I hope this helps. Thanks.

@setharnold

setharnold Nov 17, 2017

The #diff-3578ac7.. link above looks like it may have the same race condition.

I don't know Java particularly well any longer, so it's hard to suggest a replacement. Two possibilities come to mind:

  • set the umask to 0077 (octal), create the file, set the modes, and then reset umask to whatever the user wanted
  • chmod 0700 (octal) the containing directory, create the file, set the file modes, then reset the directory to whatever permissions it should have.

I hope this helps. Thanks.

This comment has been minimized.

@huntc

huntc Nov 20, 2017

How about: create the file with some temporary name/location, do what you need to do and then move it. file moves are atomic.

@huntc

huntc Nov 20, 2017

How about: create the file with some temporary name/location, do what you need to do and then move it. file moves are atomic.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment