Skip to content

hardening: fix #18995 #28029

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.

Sign up for GitHub

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

Merged
Ericson2314 merged 14 commits into from
Apr 10, 2018
Merged

hardening: fix #18995 #28029

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
9fe17b2
hardening: fix #18995
cstrahan Aug 8, 2017
9783a67
hardening: use lib.toList
cstrahan Aug 8, 2017
0937df4
hardening: fix bug/typo
cstrahan Aug 8, 2017
9920923
hardening: fix careless bugs
cstrahan Aug 8, 2017
cc7ce57
hardening: clarify the whitelist logic
cstrahan Mar 6, 2018
fc46895
hardening: allow user supplied flags to override
cstrahan Mar 6, 2018
634c748
hardening: initial cross support
cstrahan Mar 6, 2018
806edaa
hardening: ld wrapper changes, setup-hook, etc
cstrahan Mar 7, 2018
273ce83
hardening: make requested fixes
cstrahan Apr 10, 2018
386e77d
hardening: simplify reporting of disabled flags
cstrahan Apr 10, 2018
4c76d87
hardenning: Rejigger ifs and explicit declare and unset -v
Ericson2314 Apr 10, 2018
2364c22
hardening: line order, spacing, and pointless quoting for consistency
Ericson2314 Apr 10, 2018
21818ae
hardening: Tiny reindent
Ericson2314 Apr 10, 2018
ac4d74b
hardening: Reindent
Ericson2314 Apr 10, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 3 additions & 2 deletions pkgs/build-support/bintools-wrapper/add-flags.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ var_templates_list=(
NIX+LDFLAGS_BEFORE
NIX+LDFLAGS_AFTER
NIX+LDFLAGS_HARDEN
NIX+HARDENING_ENABLE
)
var_templates_bool=(
NIX+SET_BUILD_ID
Expand All @@ -23,10 +24,10 @@ if [ "${NIX_BINTOOLS_WRAPPER_@infixSalt@_TARGET_TARGET:-}" ]; then
fi

for var in "${var_templates_list[@]}"; do
mangleVarList "$var" "${role_infixes[@]}"
mangleVarList "$var" ${role_infixes[@]+"${role_infixes[@]}"}
done
for var in "${var_templates_bool[@]}"; do
mangleVarBool "$var" "${role_infixes[@]}"
mangleVarBool "$var" ${role_infixes[@]+"${role_infixes[@]}"}
Copy link
Member

@Ericson2314 Ericson2314 Mar 6, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did you copy the + thing or did you find it was needed? No bash can tell the difference between empty and undefined arrays. Old bash needs your thing with set -u for empty and undefined alike, but new bash doesn't for empty and undefined alike.

Copy link
Contributor Author

@cstrahan cstrahan Mar 6, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, got errors due to role_infixes being undefined. Happened while building linuxHeaders, though might have been poked due to that in combination with some other element I'm missing. But yeah, this line failed otherwise.

Copy link
Member

@Ericson2314 Ericson2314 Mar 7, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK sounds good.

Copy link
Member

@dtzWill dtzWill Mar 13, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Err is this a thing or did you mean ":+" instead of "+"?

Copy link
Contributor Author

@cstrahan cstrahan Mar 14, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dtzWill Yeah. Per the docs:

When not performing substring expansion, using the form described below (e.g., ‘:-’), Bash tests for a parameter that is unset or null. Omitting the colon results in a test only for a parameter that is unset. Put another way, if the colon is included, the operator tests for both parameter’s existence and that its value is not null; if the colon is omitted, the operator tests only for existence.

https://www.gnu.org/software/bash/manual/html_node/Shell-Parameter-Expansion.html#Shell-Parameter-Expansion

Copy link
Member

@dtzWill dtzWill Mar 14, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TIL! Thanks! 👍

done

if [ -e @out@/nix-support/libc-ldflags ]; then
Expand Down
87 changes: 46 additions & 41 deletions pkgs/build-support/bintools-wrapper/add-hardening.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,53 +1,58 @@
hardeningFlags=(relro bindnow)
# Intentionally word-split in case 'hardeningEnable' is defined in
# Nix. Also, our bootstrap tools version of bash is old enough that
# undefined arrays trip `set -u`.
if [[ -v hardeningEnable[@] ]]; then
hardeningFlags+=(${hardeningEnable[@]})
fi
hardeningLDFlags=()
declare -a hardeningLDFlags=()

declare -A hardeningEnableMap=()

declare -A hardeningDisableMap
# Intentionally word-split in case 'NIX_HARDENING_ENABLE' is defined in Nix. The
# array expansion also prevents undefined variables from causing trouble with
# `set -u`.
for flag in ${NIX_@infixSalt@_HARDENING_ENABLE-}; do
hardeningEnableMap["$flag"]=1
done

# Intentionally word-split in case 'hardeningDisable' is defined in Nix.
for flag in ${hardeningDisable[@]:-IGNORED_KEY} @hardening_unsupported_flags@
do
hardeningDisableMap[$flag]=1
# Remove unsupported flags.
for flag in @hardening_unsupported_flags@; do
unset -v hardeningEnableMap["$flag"]
done

if (( "${NIX_DEBUG:-0}" >= 1 )); then
declare -a allHardeningFlags=(pie relro bindnow)
declare -A hardeningDisableMap=()

# Determine which flags were effectively disabled so we can report below.
for flag in "${allHardeningFlags[@]}"; do
if [[ -z "${hardeningEnableMap[$flag]-}" ]]; then
hardeningDisableMap[$flag]=1
fi
done

printf 'HARDENING: disabled flags:' >&2
(( "${#hardeningDisableMap[@]}" )) && printf ' %q' "${!hardeningDisableMap[@]}" >&2
echo >&2
fi

if [[ -z "${hardeningDisableMap[all]:-}" ]]; then
if (( "${NIX_DEBUG:-0}" >= 1 )); then
if (( "${#hardeningEnableMap[@]}" )); then
echo 'HARDENING: Is active (not completely disabled with "all" flag)' >&2;
fi
for flag in "${hardeningFlags[@]}"
do
if [[ -z "${hardeningDisableMap[$flag]:-}" ]]; then
case $flag in
pie)
if [[ ! ("$*" =~ " -shared " || "$*" =~ " -static ") ]]; then
if (( "${NIX_DEBUG:-0}" >= 1 )); then echo HARDENING: enabling LDFlags -pie >&2; fi
hardeningLDFlags+=('-pie')
fi
;;
relro)
if (( "${NIX_DEBUG:-0}" >= 1 )); then echo HARDENING: enabling relro >&2; fi
hardeningLDFlags+=('-z' 'relro')
;;
bindnow)
if (( "${NIX_DEBUG:-0}" >= 1 )); then echo HARDENING: enabling bindnow >&2; fi
hardeningLDFlags+=('-z' 'now')
;;
*)
# Ignore unsupported. Checked in Nix that at least *some*
# tool supports each flag.
;;
esac
fi
done
fi

for flag in "${!hardeningEnableMap[@]}"; do
case $flag in
pie)
if [[ ! ("$*" =~ " -shared " || "$*" =~ " -static ") ]]; then
if (( "${NIX_DEBUG:-0}" >= 1 )); then echo HARDENING: enabling LDFlags -pie >&2; fi
hardeningLDFlags+=('-pie')
fi
;;
relro)
if (( "${NIX_DEBUG:-0}" >= 1 )); then echo HARDENING: enabling relro >&2; fi
hardeningLDFlags+=('-z' 'relro')
;;
bindnow)
if (( "${NIX_DEBUG:-0}" >= 1 )); then echo HARDENING: enabling bindnow >&2; fi
hardeningLDFlags+=('-z' 'now')
;;
*)
# Ignore unsupported. Checked in Nix that at least *some*
# tool supports each flag.
;;
esac
done
4 changes: 2 additions & 2 deletions pkgs/build-support/bintools-wrapper/ld-wrapper.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,8 +57,8 @@ fi

source @out@/nix-support/add-hardening.sh

extraAfter=("${hardeningLDFlags[@]}")
extraBefore=()
extraAfter=()
Copy link
Member

@Ericson2314 Ericson2314 May 24, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@oxij @FRidh if we do all of this PR but this part, we should have the same behavior as before. That seems like a good start to me

Copy link
Member

@FRidh FRidh May 24, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds good.

Copy link
Member

@Ericson2314 Ericson2314 May 24, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I really need to go to sleep so feel free to try it.

Copy link
Contributor

@Ralith Ralith May 24, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can the hardening flags be injected at the start instead of the end? I'd hate to see us reintroduce the problems that prompted this PR in the first place.

extraBefore=(${hardeningLDFlags[@]+"${hardeningLDFlags[@]}"})

if [ -z "${NIX_@infixSalt@_LDFLAGS_SET:-}" ]; then
extraAfter+=($NIX_@infixSalt@_LDFLAGS)
Expand Down
4 changes: 4 additions & 0 deletions pkgs/build-support/bintools-wrapper/setup-hook.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -83,6 +83,10 @@ do
fi
done

# If unset, assume the default hardening flags.
: ${NIX_HARDENING_ENABLE="fortify stackprotector pic strictoverflow format relro bindnow"}
export NIX_HARDENING_ENABLE

# No local scope in sourced file
unset -v role_pre role_post cmd upper_case
set +u
4 changes: 2 additions & 2 deletions pkgs/build-support/cc-wrapper/add-flags.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,10 +31,10 @@ fi
# We need to mangle names for hygiene, but also take parameters/overrides
# from the environment.
for var in "${var_templates_list[@]}"; do
mangleVarList "$var" "${role_infixes[@]}"
mangleVarList "$var" ${role_infixes[@]+"${role_infixes[@]}"}
done
for var in "${var_templates_bool[@]}"; do
mangleVarBool "$var" "${role_infixes[@]}"
mangleVarBool "$var" ${role_infixes[@]+"${role_infixes[@]}"}
done

# `-B@out@/bin' forces cc to use ld-wrapper.sh when calling ld.
Expand Down
115 changes: 60 additions & 55 deletions pkgs/build-support/cc-wrapper/add-hardening.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,67 +1,72 @@
hardeningFlags=(fortify stackprotector pic strictoverflow format relro bindnow)
# Intentionally word-split in case 'hardeningEnable' is defined in
# Nix. Also, our bootstrap tools version of bash is old enough that
# undefined arrays trip `set -u`.
if [[ -v hardeningEnable[@] ]]; then
hardeningFlags+=(${hardeningEnable[@]})
fi
hardeningCFlags=()
declare -a hardeningCFlags=()

declare -A hardeningEnableMap=()

declare -A hardeningDisableMap
# Intentionally word-split in case 'NIX_HARDENING_ENABLE' is defined in Nix. The
# array expansion also prevents undefined variables from causing trouble with
# `set -u`.
for flag in ${NIX_@infixSalt@_HARDENING_ENABLE-}; do
hardeningEnableMap["$flag"]=1
done

# Intentionally word-split in case 'hardeningDisable' is defined in Nix.
for flag in ${hardeningDisable[@]:-IGNORED_KEY} @hardening_unsupported_flags@
do
hardeningDisableMap[$flag]=1
# Remove unsupported flags.
for flag in @hardening_unsupported_flags@; do
unset -v hardeningEnableMap["$flag"]
done

if (( "${NIX_DEBUG:-0}" >= 1 )); then
declare -a allHardeningFlags=(fortify stackprotector pie pic strictoverflow format)
declare -A hardeningDisableMap=()

# Determine which flags were effectively disabled so we can report below.
for flag in "${allHardeningFlags[@]}"; do
if [[ -z "${hardeningEnableMap[$flag]-}" ]]; then
hardeningDisableMap["$flag"]=1
fi
done

printf 'HARDENING: disabled flags:' >&2
(( "${#hardeningDisableMap[@]}" )) && printf ' %q' "${!hardeningDisableMap[@]}" >&2
echo >&2
fi

if [[ -z "${hardeningDisableMap[all]:-}" ]]; then
if (( "${NIX_DEBUG:-0}" >= 1 )); then
if (( "${#hardeningEnableMap[@]}" )); then
echo 'HARDENING: Is active (not completely disabled with "all" flag)' >&2;
fi
for flag in "${hardeningFlags[@]}"
do
if [[ -z "${hardeningDisableMap[$flag]:-}" ]]; then
case $flag in
fortify)
if (( "${NIX_DEBUG:-0}" >= 1 )); then echo HARDENING: enabling fortify >&2; fi
hardeningCFlags+=('-O2' '-D_FORTIFY_SOURCE=2')
;;
stackprotector)
if (( "${NIX_DEBUG:-0}" >= 1 )); then echo HARDENING: enabling stackprotector >&2; fi
hardeningCFlags+=('-fstack-protector-strong' '--param' 'ssp-buffer-size=4')
;;
pie)
if (( "${NIX_DEBUG:-0}" >= 1 )); then echo HARDENING: enabling CFlags -fPIE >&2; fi
hardeningCFlags+=('-fPIE')
if [[ ! ("$*" =~ " -shared " || "$*" =~ " -static ") ]]; then
if (( "${NIX_DEBUG:-0}" >= 1 )); then echo HARDENING: enabling LDFlags -pie >&2; fi
hardeningCFlags+=('-pie')
fi
;;
pic)
if (( "${NIX_DEBUG:-0}" >= 1 )); then echo HARDENING: enabling pic >&2; fi
hardeningCFlags+=('-fPIC')
;;
strictoverflow)
if (( "${NIX_DEBUG:-0}" >= 1 )); then echo HARDENING: enabling strictoverflow >&2; fi
hardeningCFlags+=('-fno-strict-overflow')
;;
format)
if (( "${NIX_DEBUG:-0}" >= 1 )); then echo HARDENING: enabling format >&2; fi
hardeningCFlags+=('-Wformat' '-Wformat-security' '-Werror=format-security')
;;
*)
# Ignore unsupported. Checked in Nix that at least *some*
# tool supports each flag.
;;
esac
fi
done
fi

for flag in "${!hardeningEnableMap[@]}"; do
case $flag in
fortify)
if (( "${NIX_DEBUG:-0}" >= 1 )); then echo HARDENING: enabling fortify >&2; fi
hardeningCFlags+=('-O2' '-D_FORTIFY_SOURCE=2')
;;
stackprotector)
if (( "${NIX_DEBUG:-0}" >= 1 )); then echo HARDENING: enabling stackprotector >&2; fi
hardeningCFlags+=('-fstack-protector-strong' '--param' 'ssp-buffer-size=4')
;;
pie)
if (( "${NIX_DEBUG:-0}" >= 1 )); then echo HARDENING: enabling CFlags -fPIE >&2; fi
hardeningCFlags+=('-fPIE')
if [[ ! ("$*" =~ " -shared " || "$*" =~ " -static ") ]]; then
if (( "${NIX_DEBUG:-0}" >= 1 )); then echo HARDENING: enabling LDFlags -pie >&2; fi
hardeningCFlags+=('-pie')
fi
;;
pic)
if (( "${NIX_DEBUG:-0}" >= 1 )); then echo HARDENING: enabling pic >&2; fi
hardeningCFlags+=('-fPIC')
;;
strictoverflow)
if (( "${NIX_DEBUG:-0}" >= 1 )); then echo HARDENING: enabling strictoverflow >&2; fi
hardeningCFlags+=('-fno-strict-overflow')
;;
format)
if (( "${NIX_DEBUG:-0}" >= 1 )); then echo HARDENING: enabling format >&2; fi
hardeningCFlags+=('-Wformat' '-Wformat-security' '-Werror=format-security')
;;
*)
# Ignore unsupported. Checked in Nix that at least *some*
# tool supports each flag.
;;
esac
done
4 changes: 2 additions & 2 deletions pkgs/build-support/cc-wrapper/cc-wrapper.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -134,8 +134,8 @@ fi
source @out@/nix-support/add-hardening.sh

# Add the flags for the C compiler proper.
extraAfter=($NIX_@infixSalt@_CFLAGS_COMPILE "${hardeningCFlags[@]}")
extraBefore=()
extraAfter=($NIX_@infixSalt@_CFLAGS_COMPILE)
extraBefore=(${hardeningCFlags[@]+"${hardeningCFlags[@]}"})

if [ "$dontLink" != 1 ]; then

Expand Down
4 changes: 4 additions & 0 deletions pkgs/build-support/cc-wrapper/setup-hook.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -147,6 +147,10 @@ export ${role_pre}CXX=@named_cxx@
export CC${role_post}=@named_cc@
export CXX${role_post}=@named_cxx@

# If unset, assume the default hardening flags.
: ${NIX_HARDENING_ENABLE="fortify stackprotector pic strictoverflow format relro bindnow"}
Copy link
Contributor Author

@cstrahan cstrahan Mar 7, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When I split up into two variables, this will contain just the flags that make sense for the cc wrapper. Similarly for the bintools' setup-hook.

Copy link
Contributor Author

@cstrahan cstrahan Mar 7, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(For posterity: per IRC convo, I not sure if we're going with the split variables over the unified variable approach. In light of this, the above comment might be misleading.)

export NIX_HARDENING_ENABLE

# No local scope in sourced file
unset -v role_pre role_post
set +u
7 changes: 7 additions & 0 deletions pkgs/stdenv/generic/make-derivation.nix
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -74,6 +74,11 @@ rec {
# TODO(@Ericson2314): Make this more modular, and not O(n^2).
let
supportedHardeningFlags = [ "fortify" "stackprotector" "pie" "pic" "strictoverflow" "format" "relro" "bindnow" ];
defaultHardeningFlags = lib.remove "pie" supportedHardeningFlags;
enabledHardeningOptions =
if builtins.elem "all" hardeningDisable
then []
else lib.subtractLists hardeningDisable (defaultHardeningFlags ++ hardeningEnable);
Copy link
Contributor Author

@cstrahan cstrahan Mar 7, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hopefully a little cleaner than before.

# hardeningDisable additionally supports "all".
erroneousHardeningFlags = lib.subtractLists supportedHardeningFlags (hardeningEnable ++ lib.remove "all" hardeningDisable);
Copy link
Member

@Ericson2314 Ericson2314 Apr 10, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@cstrahan does this overlap with the lib.subtractLists above?

Copy link
Contributor Author

@cstrahan cstrahan Apr 10, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Ericson2314 Overlap in what way? I think they're distinct uses (one to check for erroneous flags, the other to calculate the effective flags).

in if builtins.length erroneousHardeningFlags != 0
Expand Down Expand Up @@ -179,6 +184,8 @@ rec {
++ optional (elem "host" configurePlatforms) "--host=${stdenv.hostPlatform.config}"
++ optional (elem "target" configurePlatforms) "--target=${stdenv.targetPlatform.config}";

} // lib.optionalAttrs (hardeningDisable != [] || hardeningEnable != []) {
Copy link
Contributor

@dezgeg dezgeg Aug 8, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IIRC doing this sort of stuff on the Nix level is somewhat discouraged since it slows down evaluation of Nixpkgs.

Copy link
Member

@Ericson2314 Ericson2314 Aug 8, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, more bash causes other problems too. I don't think its so clear cut.

NIX_HARDENING_ENABLE = enabledHardeningOptions;
Copy link
Contributor Author

@cstrahan cstrahan Jan 26, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Ericson2314 Regarding your comment regarding NIX_CC_WRAPPER_@infixSalt@_HARDENING_ENABLE, what's the appropriate thing to do here? Should I mostly leave this alone (the only change, perhaps, being a rename to NIX_CC_WRAPPER_HARDENING_ENABLE), and then have (per another one of your recommendations) the setupHook for the CC-wrapper use the $NIX_CC_WRAPPER_HARDENING_ENABLE when $NIX_CC_WRAPPER_@infixSalt@_HARDENING_ENABLE isn't defined? Curious what the exact logic should be here.

Copy link
Member

@Ericson2314 Ericson2314 Jan 26, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. https://github.com/NixOS/nixpkgs/blob/master/pkgs/build-support/cc-wrapper/add-flags.sh#L7-L13 in there goes NIX_CC_WRAPPER+HARDENING_ENABLE.
  2. this line becomes NIX_CC_WRAPPER_HARDENING_ENABLE ..

Now that there's a bintools wrapper too, we would have to do an analogous thing there with this plan. So OTOH if we want just one variable with both, then maybe we shouldn't add the _CC_WRAPPER to the name. But, no matter what we still want to do the infix salt.

} // lib.optionalAttrs (stdenv.buildPlatform.isDarwin) {
# TODO: remove lib.unique once nix has a list canonicalization primitive
__sandboxProfile =
Expand Down