Patchwork [RFC] toolchain-external: instrument wrapper to warn about unsafe paths

login
register
mail settings
Submitter Thomas Petazzoni
Date Feb. 10, 2014, 11:28 p.m.
Message ID <1392074881-12508-1-git-send-email-thomas.petazzoni@free-electrons.com>
Download mbox | patch
Permalink /patch/319085/
State Superseded
Headers show

Comments

Thomas Petazzoni - Feb. 10, 2014, 11:28 p.m.
The CodeSourcery toolchains have a very interesting feature: they warn
the user when an unsafe header or library path is used, i.e a path
that will lead host headers or libraries to leak into the build.

This commit adds a similar functionality into our external toolchain
wrapper, so that it can be used with all external toolchains, and can
also be tuned as needed. By default, the external toolchain wrapper
now gives warnings such as:

  WARNING: unsafe header/library path used in cross-compilation: '-I /usr/foo'
  WARNING: unsafe header/library path used in cross-compilation: '-L /usr/bleh'

but the compilation continues successfully. One can then easily grep
in his build log to search for occurences of this message.

Optionally, if BR_PARANOID_WRAPPER is defined in the environment, the
external wrapper will instead error out and abort the compilation. We
could then one day imagine setting this BR_PARANOID_WRAPPER in the
autobuilders.

A similar change could be made to the internal toolchain backend
either by making it use a wrapper like the external toolchain one, or
by adding some patches to gcc, by borrowing the changes made by the
CodeSourcery people.

Signed-off-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
---
 .../toolchain-external/ext-toolchain-wrapper.c     | 27 ++++++++++++++++++++++
 1 file changed, 27 insertions(+)
Yann E. MORIN - Feb. 11, 2014, 12:33 a.m.
Thomas, All,

On 2014-02-11 00:28 +0100, Thomas Petazzoni spake thusly:
> The CodeSourcery toolchains have a very interesting feature: they warn
> the user when an unsafe header or library path is used, i.e a path
> that will lead host headers or libraries to leak into the build.
> 
> This commit adds a similar functionality into our external toolchain
> wrapper, so that it can be used with all external toolchains, and can
> also be tuned as needed. By default, the external toolchain wrapper
> now gives warnings such as:
> 
>   WARNING: unsafe header/library path used in cross-compilation: '-I /usr/foo'
>   WARNING: unsafe header/library path used in cross-compilation: '-L /usr/bleh'
> 
> but the compilation continues successfully. One can then easily grep
> in his build log to search for occurences of this message.
> 
> Optionally, if BR_PARANOID_WRAPPER is defined in the environment, the
> external wrapper will instead error out and abort the compilation. We
> could then one day imagine setting this BR_PARANOID_WRAPPER in the
> autobuilders.

I think I'd prefer we do as for BR_DEBUG_WRAPPER:
    BR2_PARANOID_WRAPPER undefined or empty: nothing
    BR2_PARANOID_WRAPPER=WARNING           : warning
    BR2_PARANOID_WRAPPER=ERROR             : error out

(or use numbers if you prefer)

Side-note: BR_DEBUG_WRAPPER should probably be renamed to
BR2_DEBUG_WRAPPER, to follow the newly-stated naming scheme, no?

> A similar change could be made to the internal toolchain backend
> either by making it use a wrapper like the external toolchain one, or
> by adding some patches to gcc, by borrowing the changes made by the
> CodeSourcery people.

Maybe it would be best to not duplicate this, and always use the
wrapper, even for the internal backend?

> Signed-off-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
> ---
>  .../toolchain-external/ext-toolchain-wrapper.c     | 27 ++++++++++++++++++++++
>  1 file changed, 27 insertions(+)
> 
> diff --git a/toolchain/toolchain-external/ext-toolchain-wrapper.c b/toolchain/toolchain-external/ext-toolchain-wrapper.c
> index 81c6ed1..c8dcad5 100644
> --- a/toolchain/toolchain-external/ext-toolchain-wrapper.c
> +++ b/toolchain/toolchain-external/ext-toolchain-wrapper.c
> @@ -77,6 +77,7 @@ int main(int argc, char **argv)
>  	char *progpath = argv[0];
>  	char *basename;
>  	char *env_debug;
> +	char *paranoid_wrapper;
>  	int ret, i, count = 0, debug;
>  
>  	/* Calculate the relative paths */
> @@ -178,6 +179,32 @@ int main(int argc, char **argv)
>  	}
>  #endif /* ARCH || TUNE || CPU */
>  
> +	paranoid_wrapper = getenv("BR_PARANOID_WRAPPER");
> +
> +	/* Check for unsafe library and header paths */
> +	for (i = 1; i < argc; i++) {
> +		if (!strncmp(argv[i], "-I/usr", strlen("-I/usr")) ||
> +		    !strncmp(argv[i], "-L/usr", strlen("-L/usr"))) {

No need for strncmp: you're comparing to a constant, so it will not
overflow.

Also, using strlen on an argument of strncmp is flawed to begin with.
For  strncmp(a, b, strlen(b)) :
  - if 'b' is a constant, there's no need for strncmp to begin with
  - if 'b' is a user-supplied value, strlen will happily go west, and
    will not protect strncmp anyway.

> +			fprintf(stderr, "%s: unsafe header/library path used in cross-compilation: '%s'\n",
> +				paranoid_wrapper ? "ERROR" : "WARNING", argv[i]);
> +			if (paranoid_wrapper)
> +				exit(1);
> +			continue;
> +		}
> +
> +		if (!strcmp(argv[i], "-L") || !strcmp(argv[i], "-I")) {

And here you're using strcmp.

> +			if (i == argc)
> +				continue;
> +			if (!strncmp(argv[i+1], "/usr", strlen("/usr"))) {

Ditto strncmp.

> +				fprintf(stderr, "%s: unsafe header/library path used in cross-compilation: '%s %s'\n",
> +					paranoid_wrapper ? "ERROR" : "WARNING", argv[i], argv[i + 1]);
> +				if (paranoid_wrapper)
> +					exit(1);
> +				continue;
> +			}
> +		}
> +	}
> +
>  	/* append forward args */
>  	memcpy(cur, &argv[1], sizeof(char *) * (argc - 1));
>  	cur += argc - 1;

Otherwise, I like the idea pretty much! :-)

Regards,
Yann E. MORIN.
Baruch Siach - Feb. 11, 2014, 6:21 a.m.
Hi Thomas,

On Tue, Feb 11, 2014 at 12:28:01AM +0100, Thomas Petazzoni wrote:
> The CodeSourcery toolchains have a very interesting feature: they warn
> the user when an unsafe header or library path is used, i.e a path
> that will lead host headers or libraries to leak into the build.
> 
> This commit adds a similar functionality into our external toolchain
> wrapper, so that it can be used with all external toolchains, and can
> also be tuned as needed. By default, the external toolchain wrapper
> now gives warnings such as:
> 
>   WARNING: unsafe header/library path used in cross-compilation: '-I /usr/foo'
>   WARNING: unsafe header/library path used in cross-compilation: '-L /usr/bleh'

I'd mention that this makes Buildroot builds under /usr even more problematic. 
I thought this limitation appears in the documentation, but I can't find it 
there now.

baruch
Thomas Petazzoni - Feb. 11, 2014, 8:18 a.m.
Dear Yann E. MORIN,

On Tue, 11 Feb 2014 01:33:27 +0100, Yann E. MORIN wrote:

> > Optionally, if BR_PARANOID_WRAPPER is defined in the environment, the
> > external wrapper will instead error out and abort the compilation. We
> > could then one day imagine setting this BR_PARANOID_WRAPPER in the
> > autobuilders.
> 
> I think I'd prefer we do as for BR_DEBUG_WRAPPER:
>     BR2_PARANOID_WRAPPER undefined or empty: nothing
>     BR2_PARANOID_WRAPPER=WARNING           : warning
>     BR2_PARANOID_WRAPPER=ERROR             : error out

Sure, I don't have any particular feeling about this. However, I would
maybe like to have this feature enabled as the warning level by default.

> Side-note: BR_DEBUG_WRAPPER should probably be renamed to
> BR2_DEBUG_WRAPPER, to follow the newly-stated naming scheme, no?

If I understood the newly-stated naming scheme, yes, I believe you're
right, it should be renamed BR2_DEBUG_WRAPPER.

> > A similar change could be made to the internal toolchain backend
> > either by making it use a wrapper like the external toolchain one, or
> > by adding some patches to gcc, by borrowing the changes made by the
> > CodeSourcery people.
> 
> Maybe it would be best to not duplicate this, and always use the
> wrapper, even for the internal backend?

Which is exactly one of the two solutions I proposed in the paragraph
you're quoting :-)

> 
> > Signed-off-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
> > ---
> >  .../toolchain-external/ext-toolchain-wrapper.c     | 27 ++++++++++++++++++++++
> >  1 file changed, 27 insertions(+)
> > 
> > diff --git a/toolchain/toolchain-external/ext-toolchain-wrapper.c b/toolchain/toolchain-external/ext-toolchain-wrapper.c
> > index 81c6ed1..c8dcad5 100644
> > --- a/toolchain/toolchain-external/ext-toolchain-wrapper.c
> > +++ b/toolchain/toolchain-external/ext-toolchain-wrapper.c
> > @@ -77,6 +77,7 @@ int main(int argc, char **argv)
> >  	char *progpath = argv[0];
> >  	char *basename;
> >  	char *env_debug;
> > +	char *paranoid_wrapper;
> >  	int ret, i, count = 0, debug;
> >  
> >  	/* Calculate the relative paths */
> > @@ -178,6 +179,32 @@ int main(int argc, char **argv)
> >  	}
> >  #endif /* ARCH || TUNE || CPU */
> >  
> > +	paranoid_wrapper = getenv("BR_PARANOID_WRAPPER");
> > +
> > +	/* Check for unsafe library and header paths */
> > +	for (i = 1; i < argc; i++) {
> > +		if (!strncmp(argv[i], "-I/usr", strlen("-I/usr")) ||
> > +		    !strncmp(argv[i], "-L/usr", strlen("-L/usr"))) {
> 
> No need for strncmp: you're comparing to a constant, so it will not
> overflow.

I need strncmp here: I want to match only the first characters of
argv[i]. I.e, the above condition should match:

	-I/usr/bar
	-L/usr/local/foo
	-I/usr/include/mysql

etc. If I use strcmp(), it is going to compare the entire string, and
-I/usr/include/mysql is not the same as -I/usr.

> Also, using strlen on an argument of strncmp is flawed to begin with.

Weird, a certain Yann E. Morin did exactly this in
http://git.buildroot.net/buildroot/commit/toolchain/toolchain-external/ext-toolchain-wrapper.c?id=2c1dc32647eb308126b0ae80a91988059d39aa7b :-)

> For  strncmp(a, b, strlen(b)) :
>   - if 'b' is a constant, there's no need for strncmp to begin with
>   - if 'b' is a user-supplied value, strlen will happily go west, and
>     will not protect strncmp anyway.

You miss the case where you use strncmp() to only match the first N
characters of the string. Which is both what I'm doing here, and what
your patch for -march, -mtune and -mcpu is doing.

> > +			fprintf(stderr, "%s: unsafe header/library path used in cross-compilation: '%s'\n",
> > +				paranoid_wrapper ? "ERROR" : "WARNING", argv[i]);
> > +			if (paranoid_wrapper)
> > +				exit(1);
> > +			continue;
> > +		}
> > +
> > +		if (!strcmp(argv[i], "-L") || !strcmp(argv[i], "-I")) {
> 
> And here you're using strcmp.

Yes, because I'm matching the entire string.

> 
> > +			if (i == argc)
> > +				continue;
> > +			if (!strncmp(argv[i+1], "/usr", strlen("/usr"))) {
> 
> Ditto strncmp.

Yes because I want to match only the beginning of the string, so that
this condition together with the above matches:

	-I /usr/include/mysql
	-L /usr/foo

> Otherwise, I like the idea pretty much! :-)

Me too :)

Best regards,

Thomas
Thomas Petazzoni - Feb. 11, 2014, 8:24 a.m.
Dear Baruch Siach,

On Tue, 11 Feb 2014 08:21:40 +0200, Baruch Siach wrote:

> On Tue, Feb 11, 2014 at 12:28:01AM +0100, Thomas Petazzoni wrote:
> > The CodeSourcery toolchains have a very interesting feature: they warn
> > the user when an unsafe header or library path is used, i.e a path
> > that will lead host headers or libraries to leak into the build.
> > 
> > This commit adds a similar functionality into our external toolchain
> > wrapper, so that it can be used with all external toolchains, and can
> > also be tuned as needed. By default, the external toolchain wrapper
> > now gives warnings such as:
> > 
> >   WARNING: unsafe header/library path used in cross-compilation: '-I /usr/foo'
> >   WARNING: unsafe header/library path used in cross-compilation: '-L /usr/bleh'
> 
> I'd mention that this makes Buildroot builds under /usr even more problematic. 

Yes, this is true. Technically speaking, testing for -I/usr or -L/usr
is not the ideal way to achieve this. The ideal way would be to look if
only headers/libraries from the toolchain sysroot, and from the package
source tree are used. But this is fairly hard to achieve,
unfortunately. I am open to suggestions on how to achieve this.

But in any case, this mechanism will have to have a mechanism to be
entirely disabled.

> I thought this limitation appears in the documentation, but I can't find it 
> there now.

I don't think it's written in the documentation, but we have a bug
report for it, at https://bugs.busybox.net/show_bug.cgi?id=5750.

Best regards,

Thomas
Yann E. MORIN - Feb. 11, 2014, 5:53 p.m.
Thomas, All,

On 2014-02-11 09:18 +0100, Thomas Petazzoni spake thusly:
> On Tue, 11 Feb 2014 01:33:27 +0100, Yann E. MORIN wrote:
> 
> > > Optionally, if BR_PARANOID_WRAPPER is defined in the environment, the
> > > external wrapper will instead error out and abort the compilation. We
> > > could then one day imagine setting this BR_PARANOID_WRAPPER in the
> > > autobuilders.
> > 
> > I think I'd prefer we do as for BR_DEBUG_WRAPPER:
> >     BR2_PARANOID_WRAPPER undefined or empty: nothing
> >     BR2_PARANOID_WRAPPER=WARNING           : warning
> >     BR2_PARANOID_WRAPPER=ERROR             : error out
> 
> Sure, I don't have any particular feeling about this. However, I would
> maybe like to have this feature enabled as the warning level by default.

Probably sane, indeed.

> > Side-note: BR_DEBUG_WRAPPER should probably be renamed to
> > BR2_DEBUG_WRAPPER, to follow the newly-stated naming scheme, no?
> 
> If I understood the newly-stated naming scheme, yes, I believe you're
> right, it should be renamed BR2_DEBUG_WRAPPER.

Ok, will cook a patch.

> > > A similar change could be made to the internal toolchain backend
> > > either by making it use a wrapper like the external toolchain one, or
> > > by adding some patches to gcc, by borrowing the changes made by the
> > > CodeSourcery people.
> > 
> > Maybe it would be best to not duplicate this, and always use the
> > wrapper, even for the internal backend?
> 
> Which is exactly one of the two solutions I proposed in the paragraph
> you're quoting :-)

And I was just expressing my preference. ;-)

> > > Signed-off-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
> > > ---
> > >  .../toolchain-external/ext-toolchain-wrapper.c     | 27 ++++++++++++++++++++++
> > >  1 file changed, 27 insertions(+)
> > > 
> > > diff --git a/toolchain/toolchain-external/ext-toolchain-wrapper.c b/toolchain/toolchain-external/ext-toolchain-wrapper.c
> > > index 81c6ed1..c8dcad5 100644
> > > --- a/toolchain/toolchain-external/ext-toolchain-wrapper.c
> > > +++ b/toolchain/toolchain-external/ext-toolchain-wrapper.c
> > > @@ -77,6 +77,7 @@ int main(int argc, char **argv)
> > >  	char *progpath = argv[0];
> > >  	char *basename;
> > >  	char *env_debug;
> > > +	char *paranoid_wrapper;
> > >  	int ret, i, count = 0, debug;
> > >  
> > >  	/* Calculate the relative paths */
> > > @@ -178,6 +179,32 @@ int main(int argc, char **argv)
> > >  	}
> > >  #endif /* ARCH || TUNE || CPU */
> > >  
> > > +	paranoid_wrapper = getenv("BR_PARANOID_WRAPPER");
> > > +
> > > +	/* Check for unsafe library and header paths */
> > > +	for (i = 1; i < argc; i++) {
> > > +		if (!strncmp(argv[i], "-I/usr", strlen("-I/usr")) ||
> > > +		    !strncmp(argv[i], "-L/usr", strlen("-L/usr"))) {

Don't we also want to check -L/lib as well?

> > No need for strncmp: you're comparing to a constant, so it will not
> > overflow.
> 
> I need strncmp here: I want to match only the first characters of
> argv[i]. I.e, the above condition should match:
> 
> 	-I/usr/bar
> 	-L/usr/local/foo
> 	-I/usr/include/mysql
> 
> etc. If I use strcmp(), it is going to compare the entire string, and
> -I/usr/include/mysql is not the same as -I/usr.

Doh, righto-right. I should not review patches so late in the night...

> > Also, using strlen on an argument of strncmp is flawed to begin with.
> 
> Weird, a certain Yann E. Morin did exactly this in
> http://git.buildroot.net/buildroot/commit/toolchain/toolchain-external/ext-toolchain-wrapper.c?id=2c1dc32647eb308126b0ae80a91988059d39aa7b :-)

Hehe! ;-)

Regards,
Yann E. MORIN.

Patch

diff --git a/toolchain/toolchain-external/ext-toolchain-wrapper.c b/toolchain/toolchain-external/ext-toolchain-wrapper.c
index 81c6ed1..c8dcad5 100644
--- a/toolchain/toolchain-external/ext-toolchain-wrapper.c
+++ b/toolchain/toolchain-external/ext-toolchain-wrapper.c
@@ -77,6 +77,7 @@  int main(int argc, char **argv)
 	char *progpath = argv[0];
 	char *basename;
 	char *env_debug;
+	char *paranoid_wrapper;
 	int ret, i, count = 0, debug;
 
 	/* Calculate the relative paths */
@@ -178,6 +179,32 @@  int main(int argc, char **argv)
 	}
 #endif /* ARCH || TUNE || CPU */
 
+	paranoid_wrapper = getenv("BR_PARANOID_WRAPPER");
+
+	/* Check for unsafe library and header paths */
+	for (i = 1; i < argc; i++) {
+		if (!strncmp(argv[i], "-I/usr", strlen("-I/usr")) ||
+		    !strncmp(argv[i], "-L/usr", strlen("-L/usr"))) {
+			fprintf(stderr, "%s: unsafe header/library path used in cross-compilation: '%s'\n",
+				paranoid_wrapper ? "ERROR" : "WARNING", argv[i]);
+			if (paranoid_wrapper)
+				exit(1);
+			continue;
+		}
+
+		if (!strcmp(argv[i], "-L") || !strcmp(argv[i], "-I")) {
+			if (i == argc)
+				continue;
+			if (!strncmp(argv[i+1], "/usr", strlen("/usr"))) {
+				fprintf(stderr, "%s: unsafe header/library path used in cross-compilation: '%s %s'\n",
+					paranoid_wrapper ? "ERROR" : "WARNING", argv[i], argv[i + 1]);
+				if (paranoid_wrapper)
+					exit(1);
+				continue;
+			}
+		}
+	}
+
 	/* append forward args */
 	memcpy(cur, &argv[1], sizeof(char *) * (argc - 1));
 	cur += argc - 1;