Fix flags attributions

[ymusachio]

The change was necessary to handle the LDFLAGS and CFLAGS flags.
04-fix-flags.md

[msharov]

That will break the custom CFLAGS functionality, not enable it. When you use custom CFLAGS, it is usually in order to override a flag that Config.mk already sets. If Config.mk were to update CFLAGS using +=, your custom flags will be first on the command line and will be overridden.

The current syntax already lets you override CFLAGS with make CFLAGS=yourflags or with make -e.

[ymusachio]

Hey @msharov , nice to meet you!

Well, I'm not a developer, so maybe I can sin in some solutions... Anyway, the intention is to perform these Flags treatments:

LDFLAGS = -fPIE -pie
CFLAGS = -fPIE -D_FORTIFY_SOURCE=2

With the knowledge you have, what would be the best solution for this? Or maybe this treatment is not valid for the code?

[msharov]

There are three ways to do it.
You can pass the flags directly to make on the command line:

make LDFLAGS="-fPIE -pie" CFLAGS="-fPIE -D_FORTIFY_SOURCE=2"

You can have the flags in the environment and use the -e flag:

export LDFLAGS = -fPIE -pie
export CFLAGS = -fPIE -D_FORTIFY_SOURCE=2
make -e

Finally, you can add extra substitutions to config.status:

#!/bin/sh
./configure --prefix=/usr
sed -i 's/:= -s/:= -s -fPIE -pie/;s/-pedantic/-pedantic -fPIE -D_FORTIFY_SOURCE=2/' Config.mk

This is what I do when I want custom flags in the projects I'm actively working on. The configure script will keep the tail contents of config.status, so you can rerun configure with different flags or run make as often as you want and the substitutions will happen automatically.

The one way I intentionally do not support is having configure substitute CFLAGS and LDFLAGS from the environment. Most people have no idea what is in their shell environment and having this happen would be unexpected and usually undesirable. Setting build flags should be an intentional act, because, once again, most people should not do it.

[msharov]

Now, responding to your second question, PIE and _FORTIFY_SOURCE will not help make berry more secure. Hackers have long been able to hack relocatable executables, so it makes no difference if they are or not. You might lock out some noob script kiddies, but even they would know where to get the code to get around your PIE.

_FORTIFY_SOURCE mainly protects against static buffer overflows. To overflow buffers you'd need to find code that puts some user-controlled data into a static buffer. In berry, the only user-supplied data is the commands given to berryc. If you look in client.c, you'll find out that the commands are looked up in a table and no user data is ever copied into any static buffer. Thus, all you accomplish with _FORTIFY_SOURCE is add unnecessary overhead.

Even if there were instances of static buffers that could overflow, the right thing to do in that case would be to fix the problem. If you are not a programmer, find someone who is and is willing to do a security review. _FORTIFY_SOURCE is just a band-aid and for only one of many possible sources of security problems. It is not a magic spell of hacker protection.

[ymusachio]

@msharov , hi!
I recently opted to package Berry for Debian, and during the packaging process there are some treatments that it is advisable to do in the package. Of course, these treatments are not mandatory, but advisable, as they bring more security (in general) to the package.
Your explanation of whether or not to include handling these flags in Berry, I understand. Anyway, I decided to treat them in my package.
I'm uploading a patch for this treatment here (with the changes you said would be a better option for this), so you can review it (if possible, of course). :)

PS: This patch is for my packaging, not Berry itself.

04-fix-flags.md

[msharov]

I would recommend putting the changes into config.status as I described above, instead of modifying Config.mk.in. Otherwise, I'm not going to tell you what flags to use in your own package. I'll merely suggest that the patch for your package should not be a berry pull request.

[ymusachio]

@msharov
It wasn't so clear to me what the difference would be between putting this flag information in Config.status instead of Config.mk.in . Can you explain to me?

[msharov]

The difference is that Config.mk.in is part of the project, while config.status contains build configuration. Changing compiler flags is part of build configuration specific to your platform, and so goes in the latter. Patches to project files are for fixing bugs in the project, and you are not doing that.