Closed Bug 1418052 Opened 7 years ago Closed 6 years ago

Disable FORTIFY_SOURCE if --disable-hardening is set

Categories

(Firefox Build System :: General, enhancement)

Product:
Firefox Build System
Component:
General
Type:
enhancement
Priority:
Not set
Severity:
normal

Tracking

(firefox59 fixed)

Status:
RESOLVED FIXED
Milestone:
mozilla59
Tracking Flags:
Tracking Status
firefox59 --- fixed

People

(Reporter: tjr, Assigned: tjr)

References

Details

(Whiteboard: [tor 21925])

Attachments

(1 file, 1 obsolete file)

Bug 1418052 Correctly process hardening flags for ASAN, --disable-hardening, and --disable-optimize
59 bytes, text/x-review-board-request
decoder
: review+
glandium
: review+
Details 
      No description provided.
See Also: → 1418398
Comment on attachment 8930362 [details]
Bug 1418052 Correctly process hardening flags for ASAN, --disable-hardening, and --disable-optimize

https://reviewboard.mozilla.org/r/201502/#review207240

At this point, it seems it would be less convoluted to set MOZ_HARDENING_FLAGS to whatever you want, including _FORTIFY_SOURCE, in toolchain.configure, and add it to C*FLAGS in old-configure (or even through CompileFlags).
Attachment #8930362 - Flags: review?(mh+mozilla)
clearing review flags i want to refactor this actually.
Attachment #8930362 - Flags: review?(mh+mozilla)
Attachment #8930362 - Flags: review?(choller)
Okay, I think it's ready for review now.
Comment on attachment 8930362 [details]
Bug 1418052 Correctly process hardening flags for ASAN, --disable-hardening, and --disable-optimize

https://reviewboard.mozilla.org/r/201502/#review209310

Looks good to me as long as it doesn't break any builds.
Comment on attachment 8930362 [details]
Bug 1418052 Correctly process hardening flags for ASAN, --disable-hardening, and --disable-optimize

https://reviewboard.mozilla.org/r/201502/#review209534
Attachment #8930362 - Flags: review?(choller) → review+
Comment on attachment 8930362 [details]
Bug 1418052 Correctly process hardening flags for ASAN, --disable-hardening, and --disable-optimize

https://reviewboard.mozilla.org/r/201502/#review210066

::: build/moz.configure/toolchain.configure:1297
(Diff revision 4)
>         help='Enables security hardening compiler options')
>  
>  
> -@depends('--enable-hardening', c_compiler)
> -def security_hardening_cflags(value, c_compiler):
> -    if value and c_compiler.type in ['gcc', 'clang']:
> +@depends('--enable-hardening', '--enable-address-sanitizer',
> +         '--enable-optimize', c_compiler, target)
> +def security_hardening_cflags(hardening_flag, asan_flag, optimize_flag, c_compiler, target):

you might as well rename the arguments to asan and optimize, and not create local variables that just copy them.

::: build/moz.configure/toolchain.configure:1298
(Diff revision 4)
>  
>  
> -@depends('--enable-hardening', c_compiler)
> -def security_hardening_cflags(value, c_compiler):
> -    if value and c_compiler.type in ['gcc', 'clang']:
> -        return '-fstack-protector-strong'
> +@depends('--enable-hardening', '--enable-address-sanitizer',
> +         '--enable-optimize', c_compiler, target)
> +def security_hardening_cflags(hardening_flag, asan_flag, optimize_flag, c_compiler, target):
> +    compiler_is_gccish = c_compiler.type in ['gcc', 'clang']

('gcc', 'clang')

::: build/moz.configure/toolchain.configure:1307
(Diff revision 4)
> +
> +    flags = []
> +    js_flags = []
>  
> +    # FORTIFY_SOURCE ------------------------------------
> +    # Enable if no --x-hardening option is passed or --enable-hardening is passed

"If hardening is explicitly enabled, or not explicitly disabled" might be clearer.

::: build/moz.configure/toolchain.configure:1320
(Diff revision 4)
> +    if asan_is_on:
> +        flags.append("-U_FORTIFY_SOURCE")
> +        js_flags.append("-U_FORTIFY_SOURCE")

if hardening is enabled, then you have -U... -D... -U... in your flags.

::: build/moz.configure/toolchain.configure:1326
(Diff revision 4)
> +    if hardening_flag:
> +        if compiler_is_gccish and not asan_is_on:

you can merge both ifs.
Attachment #8930362 - Flags: review?(mh+mozilla)
Comment on attachment 8930362 [details]
Bug 1418052 Correctly process hardening flags for ASAN, --disable-hardening, and --disable-optimize

https://reviewboard.mozilla.org/r/201502/#review210754

Addressed all but one comment:

::: build/moz.configure/toolchain.configure:1320
(Diff revision 4)
> +    if asan_is_on:
> +        flags.append("-U_FORTIFY_SOURCE")
> +        js_flags.append("-U_FORTIFY_SOURCE")

I don't think so, maybe you misread? I ran configure with --enable-hardening and -enable-address-sanitizer, and only see one -U as expected.
Comment on attachment 8930362 [details]
Bug 1418052 Correctly process hardening flags for ASAN, --disable-hardening, and --disable-optimize

https://reviewboard.mozilla.org/r/201502/#review211754

::: build/moz.configure/toolchain.configure:1311
(Diff revision 5)
> +    if hardening_flag.origin == "default" or hardening_flag:
> +        # Require optimization for FORTIFY_SOURCE. See Bug 1417452
> +        # Also, undefine it before defining it just in case a distro adds it, see Bug 1418398
> +        if compiler_is_gccish and optimize and not asan:
> +            # Don't enable FORTIFY_SOURCE on Android on the top-level, but do enable in js/
> +            if not is_android:

you might as well use the condition directly... if target.os != 'Android':
Attachment #8930362 - Flags: review?(mh+mozilla) → review+
Pushed by csabou@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/1ee72d1a9430
Correctly process hardening flags for ASAN, --disable-hardening, and --disable-optimize r=decoder,glandium
Keywords: checkin-needed
Backout by csabou@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/57d8721abda7
Backed out changeset 1ee72d1a9430 for Linux x64 asan build bustages on /builds/worker/workspace/build/src/config/recurse.mk:32 r=backout on a CLOSED TREE
My best guess is that the cause of this is because we are going from '-U_FORTIFY_SOURCE -D_FORTIFY_SOURCE=2' for this job to just '-U_FORTIFY_SOURCE' - but I have no idea why that's a problem.

I'm going to try re-arranging the code and seeing if that makes the error go away...
Sure enough, moving the function fixed it.
Flags: needinfo?(tom)
Attachment #8942405 - Flags: review?(jmuizelaar)
Comment on attachment 8942405 [details]
Bug 1418052 Used the wrong form for add_old_configure_assignment

I spoke to soon (was looking at the wrong job)
Attachment #8942405 - Flags: review?(jmuizelaar)
Attachment #8942405 - Flags: review?(jmuizelaar) → review?(mh+mozilla)
Okay the problem was actually that I my initial attempt at add_old_configure_assignment was incorrect: the function needs to be written to return True (not just the val) for the old-configure assignment to be set. Without MOZ_ASAN we weren't disabling the sandbox, leading to weird compilation errors.

glandium - could you review my change? If it looks good I will fold it into the original commit, do a full try run, and then checkin just a single patch.
Comment on attachment 8942405 [details]
Bug 1418052 Used the wrong form for add_old_configure_assignment

https://reviewboard.mozilla.org/r/212708/#review218678

::: build/moz.configure/toolchain.configure
(Diff revision 2)
>  # Security Hardening
>  # ==============================================================
>  
>  js_option('--enable-address-sanitizer', help='Enable Address Sanitizer')
>  
> -

Removing this line will likely make some linter barf.

::: build/moz.configure/toolchain.configure:1302
(Diff revision 2)
>  # ==============================================================
>  
>  js_option('--enable-address-sanitizer', help='Enable Address Sanitizer')
>  
> -
>  @depends('--enable-address-sanitizer')

you could use @depends_if here and not need that if in the function.
Attachment #8942405 - Flags: review?(mh+mozilla)
Comment on attachment 8942405 [details]
Bug 1418052 Used the wrong form for add_old_configure_assignment

https://reviewboard.mozilla.org/r/212708/#review219602
Attachment #8942405 - Flags: review?(mh+mozilla) → review+
Attachment #8942405 - Attachment is obsolete: true
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/1578425fbe9f
Correctly process hardening flags for ASAN, --disable-hardening, and --disable-optimize r=decoder,glandium
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/1578425fbe9f
Status: NEW → RESOLVED
Closed: 6 years ago
status-firefox59: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
Whiteboard: [tor 21925]
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: