Skip to content
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.

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

-Werror=format-security #680

Closed
JimPanic opened this issue May 18, 2012 · 14 comments
Closed

-Werror=format-security #680

JimPanic opened this issue May 18, 2012 · 14 comments

Comments

@JimPanic
Copy link

When building nokogiri's native extensions with GCC 4.6.3 on Ubuntu and -Werror=format-security enabled, this error appears:

1 a.panek@test-web01 /srv/web/esotericsystems.at/email (git)-[master] % cat /usr/local/rvm/gems/ruby-1.9.3-p0/gems/nokogiri-1.5.2/ext/nokogiri/gem_make.out
/usr/local/rvm/rubies/ruby-1.9.3-p0/bin/ruby extconf.rb
extconf.rb:10: Use RbConfig instead of obsolete and deprecated Config.
checking for libxml/parser.h... yes
checking for libxslt/xslt.h... yes
checking for libexslt/exslt.h... yes
checking for iconv_open() in iconv.h... yes
checking for xmlParseDoc() in -lxml2... yes
checking for xsltParseStylesheetDoc() in -lxslt... yes
checking for exsltFuncRegister() in -lexslt... yes
checking for xmlHasFeature()... yes
checking for xmlFirstElementChild()... yes
checking for xmlRelaxNGSetParserStructuredErrors()... yes
checking for xmlRelaxNGSetParserStructuredErrors()... yes
checking for xmlRelaxNGSetValidStructuredErrors()... yes
checking for xmlSchemaSetValidStructuredErrors()... yes
checking for xmlSchemaSetParserStructuredErrors()... yes
creating Makefile

make
compiling xml_cdata.c
compiling xslt_stylesheet.c
compiling xml_element_decl.c
compiling xml_node_set.c
xml_node_set.c: In function ‘dealloc_namespace’:
xml_node_set.c:9:13: warning: cast discards ‘__attribute__((const))’ qualifier from pointer target type [-Wcast-qual]
xml_node_set.c:11:13: warning: cast discards ‘__attribute__((const))’ qualifier from pointer target type [-Wcast-qual]
compiling xml_io.c
xml_io.c: In function ‘io_read_callback’:
xml_io.c:28:3: warning: conversion to ‘int’ from ‘size_t’ may alter its value [-Wconversion]
compiling xml_attr.c
compiling xml_reader.c
compiling xml_entity_decl.c
compiling html_entity_lookup.c
compiling xml_entity_reference.c
compiling html_sax_parser_context.c
compiling xml_element_content.c
compiling xml_xpath_context.c
xml_xpath_context.c: In function ‘xpath_generic_exception_handler’:
xml_xpath_context.c:189:3: error: format not a string literal and no format arguments [-Werror=format-security]
cc1: some warnings being treated as errors
make: *** [xml_xpath_context.o] Error 1
@flavorjones
Copy link
Member

Hello!

Thank you for reporting this. Is it really an issue, though? What's actionable? Have you looked at the code? Do you have recommendations on how to fix? Why is "Werror=format-security" important to you in this context? Wouldn't this be a discussion better had on nokogiri-talk?

@JimPanic
Copy link
Author

Hello,

github was just the first communication channel I found suitable for this "bug" report. I didn't know about nokogiri-talk. :)

-Werror=format-security is turned on by default on Ubuntu since version 8.10; here's what (my) man gcc says:

-Wformat-security

[...]

NOTE: In Ubuntu 8.10 and later versions this option is enabled by default for C, C++, ObjC, ObjC++.
To disable, use -Wno-format-security, or disable all format warnings with -Wformat=0. To make
format security warnings fatal, specify -Werror=format-security.

The error points to line 189 in xml_xpath_context.c, but I suppose it's vasprintf in line 190 causing this warning/error. Searching for fixes to the format-security warning, I've found supplying a format "%s" as string literal in a few projects.

Currently, vasprintf is called as such: vasprintf(&message, msg, args);, whereas msg is the message passed from libxml to the error handler. The second parameter shouldn't be the message itself, but a format string (literal).

I think this might work:

va_start(args, ctx);
vasprintf(&message, "%s", args);

Note: I haven't tested this yet, as I just built it without -Werror=format-security for now. I wanted to get the word out about this, anyways.

Should I post this on nokogiri-talk, too?

Cheers, Alex

@flavorjones
Copy link
Member

There is a difference between

-Wformat-security

which emits a compiler warning, and

-Werror=format-security

which causes the compiler to fail with a fatal error.

Ubuntu turns on warnings by default, not errors. So it's not apparent to me why you're failing above; unless you've got "-Werror" set as part of your personal development setup.

In this case, the free-text format string being used is emitted by libxml2 warnings. So, why is this an issue? There is no obvious exploit for this, unless the libxml2 library has been compromised.

And, well, I would have preferred if you'd posted to the mailing list so that this conversation was transparent and available. See http://bit.ly/nokohelp for Team Nokogiri's rationale. But no biggie now.

@JimPanic
Copy link
Author

Ah, my bad. Just found out -Werror=format-security comes from Debian's hardening-wrapper.

I don't know whether this is an issue; I took hardening as applying general best-practices. Thus, reporting it here, may it be an actual error. :)

Ay ay, will post to nokogiri-talk next time.

@JimPanic
Copy link
Author

To be honest, I'm not sure myself when and how the error part comes into play.

@flavorjones
Copy link
Member

In this case, since the format string is returned by libxml2 (along with the args to the string, suitable for a call to vasprintf), it's not obvious to me that there are any security implications.

Reading the gcc man page, it doesn't seem like I can add any sort of pragma declaration to the line to tell the compiler "hey, now, this time it's OK to call vasprintf".

Though, there is an interesting other gcc argument, -Wformat-nonliteral which appears to not raise a warning if the arguments are a va_list, which is the case here.

Can you come up with any changes to ext/nokogiri/extconf.rb that changes this behavior within Nokogiri's build? Perhaps adding a -Wformat=0 or -Wno-format-security -Wformat-nonliteral would suffice? Since my system isn't hardened in the same way, I'm not immediately able to verify what might work for you.

@JimPanic
Copy link
Author

-Wformat-nonliteral on its own doesn't work, but both -Wformat-nonliteral -Wno-format-security and -Wno-error=format-security work. Both options still brag about the warning, but don't throw an error.

I've just added a seperate $CFLAGS << " -W..." line here, I guess that's the correct place.

Will also try to find out whether there's a general way to check for hardening-wrapper being used.

@yob
Copy link
Contributor

yob commented May 29, 2012

This will make installing nokogiri on all debian wheezy system difficult, it would be good to have a work around (even if it's not nokogiri's fault)

@flavorjones
Copy link
Member

@yob - Did you read the comment above where we discover that this is only an issue if you're using the hardening-wrapper? If so, then please add some context so we know what you're referring to. (Is hardening-wrapper installed by default in Wheezy?)

@yob
Copy link
Contributor

yob commented May 29, 2012

Hardened build flags is a release goal for wheezy. So they're definitely
turned on by default for building of debian packages.

However, I received this error just by running 'gem install nokogiri' on a
wheezy system today, so maybe the flags are just on by default all the time.
On May 29, 2012 9:12 PM, "Mike Dalessio" <
[email protected]>
wrote:

@yob - Did you read the comment above where we discover that this is only
an issue if you're using the hardening-wrapper? If so, then please add some
context so we know what you're referring to. (Is hardening-wrapper
installed by default in Wheezy?)


Reply to this email directly or view it on GitHub:
https://github.com/tenderlove/nokogiri/issues/680#issuecomment-5980726

@yob
Copy link
Contributor

yob commented Jun 7, 2012

It looks like mkmf.rb sets CFLAGS based on the flags that were used to build ruby (by looking up RbConfig::CONFIG["CFLAGS"])

Since MRI 1.9.3p194 on debian ruby is built with -Werror=format-security, so without hand holding the nokogiri gem won't build and install.

@yob
Copy link
Contributor

yob commented Jun 11, 2012

thanks, 1.5.4 compiles and installs as normal.

@JimPanic
Copy link
Author

thanks!

@maciejjankowski
Copy link

can someone please fix this?
I'm having troubles on buntu :/
this giri thing is fragile like a newborn :/

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants