Skip to contentSkip to conversation

libopenssl: do not leak the compiler path (reproducible builds)

PATCH New Peter Seiderer 4 participants 10 messages Oct 27, 2017 – Feb 4, 2018

lore
Share & tools
lore

Peter Seiderer

author
#0 lore raw

PATCH 1/2 libopenssl: do not leak the compiler path (reproducible builds)

Signed-off-by Peter Seiderer <[email protected]>
---
 ...roducible-build-do-not-leak-compiler-path.patch | 26 ++++++++++++++++++++++
 1 file changed, 26 insertions(+)
 create mode 100644 package/libopenssl/0003-Reproducible-build-do-not-leak-compiler-path.patch
package/libopenssl/0003-Reproducible-build-do-not-leak-compiler-path.patch new file +26 -0
new file mode 100644
index 0000000000..eff72c548a
@@ -0,0 +1,26 @@
+From 875fcad2ad84877763cba86c1265b57679b878b0 Mon Sep 17 00:00:00 2001
+From: Peter Seiderer <[email protected]>
+Date: Tue, 24 Oct 2017 16:58:32 +0200
+Subject: [PATCH] Reproducible build: do not leak compiler path
+
+Signed-off-by: Peter Seiderer <[email protected]>
+---
+ crypto/Makefile | 2 +-
+ 1 file changed, 1 insertion(+), 1 deletion(-)
+
+diff --git a/crypto/Makefile b/crypto/Makefile
+index 7869996..7e63291 100644
+--- a/crypto/Makefile
++++ b/crypto/Makefile
+@@ -55,7 +55,7 @@ top:
+ all: shared
+ 
+ buildinf.h: ../Makefile
+-	$(PERL) $(TOP)/util/mkbuildinf.pl "$(CC) $(CFLAGS)" "$(PLATFORM)" >buildinf.h
++	$(PERL) $(TOP)/util/mkbuildinf.pl "$$(basename $(CC)) $(CFLAGS)" "$(PLATFORM)" >buildinf.h
+ 
+ x86cpuid.s:	x86cpuid.pl perlasm/x86asm.pl
+ 	$(PERL) x86cpuid.pl $(PERLASM_SCHEME) $(CFLAGS) $(PROCESSOR) > $@
+-- 
+2.11.0
+
Signature · 1 line
2.14.2
author
#1 lore raw

PATCH 2/2 dhcp: disable isc assertions (reproducible builds)

The isc assertions from the bundled bind dns library are using the __FILE__ macro for debug messages (see dhcp-4.3.5/bind/bind-9.9.9-P3/lib/isc/include/isc/assertions.h).

Disabling the assertions gains:

- reproducible builds (no built time pathes in the executable)
- space saving on the target:
dhcpd: 1.9M -> 1.6M
dhcrelay: 1.6M -> 1.3M

Signed-off-by Peter Seiderer <[email protected]>
---
 package/dhcp/dhcp.mk | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)
package/dhcp/dhcp.mk +3 -1
index 45cae087aa..0bae61283a 100644
@@ -11,7 +11,9 @@ DHCP_LICENSE = ISC
 DHCP_LICENSE_FILES = LICENSE
 DHCP_CONF_ENV = \
 	CPPFLAGS='-D_PATH_DHCPD_CONF=\"/etc/dhcp/dhcpd.conf\" \
-		-D_PATH_DHCLIENT_CONF=\"/etc/dhcp/dhclient.conf\"'
+		-D_PATH_DHCLIENT_CONF=\"/etc/dhcp/dhclient.conf\"' \
+	CFLAGS='$(TARGET_CFLAGS) -DISC_CHECK_NONE=1'
+
 DHCP_CONF_OPTS = \
 	--with-randomdev=/dev/random \
 	--with-srv-lease-file=/var/lib/dhcp/dhcpd.leases \
Signature · 1 line
2.14.2
#2 lore raw

PATCH 2/2 dhcp: disable isc assertions (reproducible builds)

Hello,

Quoted 21 lines · Peter Seiderer

The isc assertions from the bundled bind dns library are using the __FILE__ macro for debug messages (see dhcp-4.3.5/bind/bind-9.9.9-P3/lib/isc/include/isc/assertions.h).

Disabling the assertions gains:

- reproducible builds (no built time pathes in the executable)
- space saving on the target:
dhcpd: 1.9M -> 1.6M
dhcrelay: 1.6M -> 1.3M

Signed-off-by Peter Seiderer <[email protected]>
---
 package/dhcp/dhcp.mk | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)
package/dhcp/dhcp.mk +0 -0
index 45cae087aa..0bae61283a 100644

package/dhcp/dhcp.mk DHCP_LICENSE = ISC · line 15

Quoted 6 lines
package/dhcp/dhcp.mk +2 -1
@@ -11,7 +11,9 @@ DHCP_LICENSE = ISC
 DHCP_LICENSE_FILES = LICENSE
 DHCP_CONF_ENV = \
 	CPPFLAGS='-D_PATH_DHCPD_CONF=\"/etc/dhcp/dhcpd.conf\" \
-		-D_PATH_DHCLIENT_CONF=\"/etc/dhcp/dhclient.conf\"'
+		-D_PATH_DHCLIENT_CONF=\"/etc/dhcp/dhclient.conf\"' \
+	CFLAGS='$(TARGET_CFLAGS) -DISC_CHECK_NONE=1'

I'm not sure about this one.

Do we want to remove assertions from production code ?

Regarding the __FILE__ problem, I think this is going to bite us on a large number of packages. So instead of a per-package solution, we should perhaps have a look at:

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=70268, proposal for a -ffile-map-prefix option to gcc

https://wiki.debian.org/ReproducibleBuilds/BuildPathProposal, also discussing a similar idea

Peter, Arnout, Yann, what do you think, especially about the assertions ?

Best regards,

Thomas

Signature · 3 lines
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
author
#3 lore raw

PATCH 2/2 dhcp: disable isc assertions (reproducible builds)

Hello Thomas, *,

Quoted 33 lines · Thomas Petazzoni

Hello,

Quoted 21 lines · Peter Seiderer

The isc assertions from the bundled bind dns library are using the __FILE__ macro for debug messages (see dhcp-4.3.5/bind/bind-9.9.9-P3/lib/isc/include/isc/assertions.h).

Disabling the assertions gains:

- reproducible builds (no built time pathes in the executable)
- space saving on the target:
dhcpd: 1.9M -> 1.6M
dhcrelay: 1.6M -> 1.3M

Signed-off-by Peter Seiderer <[email protected]>
---
 package/dhcp/dhcp.mk | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)
package/dhcp/dhcp.mk +0 -0
index 45cae087aa..0bae61283a 100644

package/dhcp/dhcp.mk DHCP_LICENSE = ISC · line 15

Quoted 6 lines
package/dhcp/dhcp.mk +2 -1
@@ -11,7 +11,9 @@ DHCP_LICENSE = ISC
 DHCP_LICENSE_FILES = LICENSE
 DHCP_CONF_ENV = \
 	CPPFLAGS='-D_PATH_DHCPD_CONF=\"/etc/dhcp/dhcpd.conf\" \
-		-D_PATH_DHCLIENT_CONF=\"/etc/dhcp/dhclient.conf\"'
+		-D_PATH_DHCLIENT_CONF=\"/etc/dhcp/dhclient.conf\"' \
+	CFLAGS='$(TARGET_CFLAGS) -DISC_CHECK_NONE=1'

I'm not sure about this one.

Quoted 1 line

Do we want to remove assertions from production code ?

I think yes, as std::assert does in case NDEBUG is set (or many other assert implementations which are disabled in release code)...

Quoted 11 lines

Regarding the __FILE__ problem, I think this is going to bite us on a large number of packages. So instead of a per-package solution, we should perhaps have a look at:

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=70268, proposal for a -ffile-map-prefix option to gcc

https://wiki.debian.org/ReproducibleBuilds/BuildPathProposal, also discussing a similar idea

Which would save the reproducible problem, but lags the complete space saving advantage...

Quoted 2 lines

Peter, Arnout, Yann, what do you think, especially about the assertions ?

Any Comments?

Regards,
Peter

Quoted 4 lines

Best regards,

Thomas

#4 lore raw

PATCH 2/2 dhcp: disable isc assertions (reproducible builds)

In reply to Thomas Petazzoni

Thomas, Peter S., All,

On 2018-01-08 21:18 +0100, Thomas Petazzoni spake thusly:

Quoted 37 lines · Peter Seiderer
Quoted 21 lines

The isc assertions from the bundled bind dns library are using the __FILE__ macro for debug messages (see dhcp-4.3.5/bind/bind-9.9.9-P3/lib/isc/include/isc/assertions.h).

Disabling the assertions gains:

- reproducible builds (no built time pathes in the executable)
- space saving on the target:
dhcpd: 1.9M -> 1.6M
dhcrelay: 1.6M -> 1.3M

Signed-off-by Peter Seiderer <[email protected]>
---
 package/dhcp/dhcp.mk | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)
package/dhcp/dhcp.mk +0 -0
index 45cae087aa..0bae61283a 100644

package/dhcp/dhcp.mk DHCP_LICENSE = ISC · line 15

Quoted 6 lines
package/dhcp/dhcp.mk +2 -1
@@ -11,7 +11,9 @@ DHCP_LICENSE = ISC
 DHCP_LICENSE_FILES = LICENSE
 DHCP_CONF_ENV = \
 	CPPFLAGS='-D_PATH_DHCPD_CONF=\"/etc/dhcp/dhcpd.conf\" \
-		-D_PATH_DHCLIENT_CONF=\"/etc/dhcp/dhclient.conf\"'
+		-D_PATH_DHCLIENT_CONF=\"/etc/dhcp/dhclient.conf\"' \
+	CFLAGS='$(TARGET_CFLAGS) -DISC_CHECK_NONE=1'

I'm not sure about this one.

Do we want to remove assertions from production code ?

Regarding the __FILE__ problem, I think this is going to bite us on a large number of packages. So instead of a per-package solution, we should perhaps have a look at:

Quoted 8 lines

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=70268, proposal for a -ffile-map-prefix option to gcc

https://wiki.debian.org/ReproducibleBuilds/BuildPathProposal, also discussing a similar idea

Peter, Arnout, Yann, what do you think, especially about the assertions ?

Assertions are supposed to be usefull during development, not during production use, so I would be fine with disabling them, yes.

At least, I would hope that no sane developer uses assertions to check for normal error conditions in normal operation mode... ;-]

Now, about reproducibility... We could at least pass -DFILE=\"\" and so on from inside our gcc-wrapper, like we already do for __DATE__ and __TIME__. Yes, that would give an empty filename in assertions and the likes, but do we care in the end?

Regards,
Yann E. MORIN.

Signature · 6 lines
.-----------------.--------------------.------------------.--------------------.
|  Yann E. MORIN  | Real-Time Embedded | /"\ ASCII RIBBON | Erics' conspiracy: |
| +33 662 376 056 | Software  Designer | \ / CAMPAIGN     |  ___               |
| +33 223 225 172 `------------.-------:  X  AGAINST      |  \e/  There is no  |
| http://ymorin.is-a-geek.org/ | _/*\_ | / \ HTML MAIL    |   v   conspiracy.  |
'------------------------------^-------^------------------^--------------------'
#5 lore raw

PATCH 2/2 dhcp: disable isc assertions (reproducible builds)

Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line

"Yann" == Yann E MORIN <[email protected]> writes:

Hi,

Quoted 8 lines
Quoted 2 lines

Peter, Arnout, Yann, what do you think, especially about the assertions ?

Assertions are supposed to be usefull during development, not during production use, so I would be fine with disabling them, yes.

At least, I would hope that no sane developer uses assertions to check for normal error conditions in normal operation mode... ;-]

Agreed, and the space savings are quite big - So committed, thanks.

Quoted 4 lines

Now, about reproducibility... We could at least pass -DFILE=\"\" and so on from inside our gcc-wrapper, like we already do for __DATE__ and __TIME__. Yes, that would give an empty filename in assertions and the likes, but do we care in the end?

It would be nicer if we could pass basename($file), but that would break with compilations with multiple source files passed to gcc or header files, so yeah, perhaps that is the best way forward.

How does E.G. Debian handle it?

Signature · 1 line
Bye, Peter Korsgaard
#6 lore raw

PATCH 2/2 dhcp: disable isc assertions (reproducible builds)

Peter, All,

On 2018-02-03 22:54 +0100, Peter Korsgaard spake thusly:

Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line

"Yann" == Yann E MORIN <[email protected]> writes:

[--SNIP--]

Quoted 10 lines
Quoted 4 lines

Now, about reproducibility... We could at least pass -DFILE=\"\" and so on from inside our gcc-wrapper, like we already do for __DATE__ and __TIME__. Yes, that would give an empty filename in assertions and the likes, but do we care in the end?

It would be nicer if we could pass basename($file), but that would break with compilations with multiple source files passed to gcc or header files, so yeah, perhaps that is the best way forward.

How does E.G. Debian handle it?

I could not find an authoritative source for this, but from what I could hint from [0] (see the part about "Qt tests and __FILE__"), they do have a patched gcc and a special dpkg which use the BUILD_PATH_PREFIX_MAP environment variable as specified iby reproducible-builds.org [1].

Also, upstream gcc has an "in-progress" upstream solution being worked on, based on BUILD_PATH_PREFIX_MAP, but they did not like it [2]...

[0] https://reproducible.alioth.debian.org/blog/posts/134/
[1] https://reproducible-builds.org/specs/build-path-prefix-map/
[2] https://gcc.gnu.org/ml/gcc-patches/2017-08/msg00232.html

Regards,
Yann E. MORIN.

Signature · 6 lines
.-----------------.--------------------.------------------.--------------------.
|  Yann E. MORIN  | Real-Time Embedded | /"\ ASCII RIBBON | Erics' conspiracy: |
| +33 662 376 056 | Software  Designer | \ / CAMPAIGN     |  ___               |
| +33 223 225 172 `------------.-------:  X  AGAINST      |  \e/  There is no  |
| http://ymorin.is-a-geek.org/ | _/*\_ | / \ HTML MAIL    |   v   conspiracy.  |
'------------------------------^-------^------------------^--------------------'
#7 lore raw

PATCH 2/2 dhcp: disable isc assertions (reproducible builds)

Quoted 22 lines
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line

"Yann" == Yann E MORIN <[email protected]> writes:

Peter, All,
On 2018-02-03 22:54 +0100, Peter Korsgaard spake thusly:

Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line

"Yann" == Yann E MORIN <[email protected]> writes:

[--SNIP--]

Quoted 10 lines
Quoted 4 lines

Now, about reproducibility... We could at least pass -DFILE=\"\" and so on from inside our gcc-wrapper, like we already do for __DATE__ and __TIME__. Yes, that would give an empty filename in assertions and the likes, but do we care in the end?

It would be nicer if we could pass basename($file), but that would break with compilations with multiple source files passed to gcc or header files, so yeah, perhaps that is the best way forward.

How does E.G. Debian handle it?

I could not find an authoritative source for this, but from what I could hint from [0] (see the part about "Qt tests and __FILE__"), they do have a patched gcc and a special dpkg which use the BUILD_PATH_PREFIX_MAP environment variable as specified iby reproducible-builds.org [1].

Quoted 6 lines

Also, upstream gcc has an "in-progress" upstream solution being worked on, based on BUILD_PATH_PREFIX_MAP, but they did not like it [2]...

[0] https://reproducible.alioth.debian.org/blog/posts/134/
[1] https://reproducible-builds.org/specs/build-path-prefix-map/
[2] https://gcc.gnu.org/ml/gcc-patches/2017-08/msg00232.html

Thanks - Looks interesting. I wasn't aware of this BUILD_PATH_PREFIX_MAP.

Signature · 1 line
Bye, Peter Korsgaard
#8 lore raw

PATCH 1/2 libopenssl: do not leak the compiler path (reproducible builds)

In reply to Peter Seiderer

Hello,

Quoted 22 lines · Peter Seiderer
Signed-off-by Peter Seiderer <[email protected]>
---
 ...roducible-build-do-not-leak-compiler-path.patch | 26 ++++++++++++++++++++++
 1 file changed, 26 insertions(+)
 create mode 100644 package/libopenssl/0003-Reproducible-build-do-not-leak-compiler-path.patch
package/libopenssl/0003-Reproducible-build-do-not-leak-compiler-path.patch new file +10 -0
new file mode 100644
index 0000000000..eff72c548a
@@ -0,0 +1,26 @@
+From 875fcad2ad84877763cba86c1265b57679b878b0 Mon Sep 17 00:00:00 2001
+From: Peter Seiderer <[email protected]>
+Date: Tue, 24 Oct 2017 16:58:32 +0200
+Subject: [PATCH] Reproducible build: do not leak compiler path
+
+Signed-off-by: Peter Seiderer <[email protected]>
+---
+ crypto/Makefile | 2 +-
+ 1 file changed, 1 insertion(+), 1 deletion(-)
+

package/libopenssl/0003-Reproducible-build-do-not-leak-compiler-path.patch line 20

Quoted 10 lines
package/libopenssl/0003-Reproducible-build-do-not-leak-compiler-path.patch new file +10 -0
+diff --git a/crypto/Makefile b/crypto/Makefile
+index 7869996..7e63291 100644
+--- a/crypto/Makefile
++++ b/crypto/Makefile
+@@ -55,7 +55,7 @@ top:
+ all: shared
+ 
+ buildinf.h: ../Makefile
+-	$(PERL) $(TOP)/util/mkbuildinf.pl "$(CC) $(CFLAGS)" "$(PLATFORM)" >buildinf.h
++	$(PERL) $(TOP)/util/mkbuildinf.pl "$$(basename $(CC)) $(CFLAGS)" "$(PLATFORM)" >buildinf.h

I hesitated a bit on this one, because after all it's our fault: we are passing an absolute path as the value of CC. If we change that to pass just the name of the compiler, then OpenSSL doesn't have a problem.

But, it really is OpenSSL choice to hardcode such compiler/flags information into the binary, so it should sanitize that before using it.

Even though I believe there's probably not much hope, could you try to submit this patch upstream?

In the mean time, I've applied to master. Thanks!

Thomas

Signature · 3 lines
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
author
#9 lore raw

PATCH 1/2 libopenssl: do not leak the compiler path (reproducible builds)

Hello Thomas,

Quoted 40 lines · Thomas Petazzoni

Hello,

Quoted 22 lines · Peter Seiderer
Signed-off-by Peter Seiderer <[email protected]>
---
 ...roducible-build-do-not-leak-compiler-path.patch | 26 ++++++++++++++++++++++
 1 file changed, 26 insertions(+)
 create mode 100644 package/libopenssl/0003-Reproducible-build-do-not-leak-compiler-path.patch
package/libopenssl/0003-Reproducible-build-do-not-leak-compiler-path.patch new file +10 -0
new file mode 100644
index 0000000000..eff72c548a
@@ -0,0 +1,26 @@
+From 875fcad2ad84877763cba86c1265b57679b878b0 Mon Sep 17 00:00:00 2001
+From: Peter Seiderer <[email protected]>
+Date: Tue, 24 Oct 2017 16:58:32 +0200
+Subject: [PATCH] Reproducible build: do not leak compiler path
+
+Signed-off-by: Peter Seiderer <[email protected]>
+---
+ crypto/Makefile | 2 +-
+ 1 file changed, 1 insertion(+), 1 deletion(-)
+

package/libopenssl/0003-Reproducible-build-do-not-leak-compiler-path.patch line 20

Quoted 10 lines
package/libopenssl/0003-Reproducible-build-do-not-leak-compiler-path.patch new file +10 -0
+diff --git a/crypto/Makefile b/crypto/Makefile
+index 7869996..7e63291 100644
+--- a/crypto/Makefile
++++ b/crypto/Makefile
+@@ -55,7 +55,7 @@ top:
+ all: shared
+ 
+ buildinf.h: ../Makefile
+-	$(PERL) $(TOP)/util/mkbuildinf.pl "$(CC) $(CFLAGS)" "$(PLATFORM)" >buildinf.h
++	$(PERL) $(TOP)/util/mkbuildinf.pl "$$(basename $(CC)) $(CFLAGS)" "$(PLATFORM)" >buildinf.h

I hesitated a bit on this one, because after all it's our fault: we are passing an absolute path as the value of CC. If we change that to pass just the name of the compiler, then OpenSSL doesn't have a problem.

Quoted 5 lines

But, it really is OpenSSL choice to hardcode such compiler/flags information into the binary, so it should sanitize that before using it.

Even though I believe there's probably not much hope, could you try to submit this patch upstream?

Finally found some spare time and submitted upstream, see [1]...

Regards,
Peter

[1] https://github.com/openssl/openssl/pull/5218

Quoted 4 lines

In the mean time, I've applied to master. Thanks!

Thomas