Patchwork insane: detect and warn about relocations in .text

login
register
mail settings
Submitter Phil Blundell
Date Oct. 3, 2012, 10:24 a.m.
Message ID <1349259854.32611.106.camel@phil-desktop>
Download mbox | patch
Permalink /patch/37669/
State New
Headers show

Comments

Phil Blundell - Oct. 3, 2012, 10:24 a.m.
Signed-off-by: Phil Blundell <pb@pbcl.net>
---
This requires qa.elf.run_objdump() so needs to be applied after the
patch which adds that function.

 meta/classes/insane.bbclass |   26 +++++++++++++++++++++++++-
 1 file changed, 25 insertions(+), 1 deletion(-)
Martin Jansa - Oct. 3, 2012, 10:31 a.m.
On Wed, Oct 03, 2012 at 11:24:12AM +0100, Phil Blundell wrote:
> Signed-off-by: Phil Blundell <pb@pbcl.net>

Can you add a bit longer description of possible issues with relocations
in .text? So that people seeing this issue will know how dangerous it is
for them?

From my understanding (after reading
http://www.gentoo.org/proj/en/hardened/pic-fix-guide.xml) it's mostly
performance issue?

Cheers,

> ---
> This requires qa.elf.run_objdump() so needs to be applied after the
> patch which adds that function.
> 
>  meta/classes/insane.bbclass |   26 +++++++++++++++++++++++++-
>  1 file changed, 25 insertions(+), 1 deletion(-)
> 
> diff --git a/meta/classes/insane.bbclass b/meta/classes/insane.bbclass
> index 5848ab4..ff35ed8 100644
> --- a/meta/classes/insane.bbclass
> +++ b/meta/classes/insane.bbclass
> @@ -108,7 +108,7 @@ def package_qa_get_machine_dict():
>  
>  
>  # Currently not being used by default "desktop"
> -WARN_QA ?= "ldflags useless-rpaths rpaths unsafe-references-in-binaries unsafe-references-in-scripts staticdev libdir"
> +WARN_QA ?= "ldflags useless-rpaths rpaths unsafe-references-in-binaries unsafe-references-in-scripts staticdev libdir textrel"
>  ERROR_QA ?= "dev-so debug-deps dev-deps debug-files arch la2 pkgconfig la perms dep-cmp"
>  
>  ALL_QA = "${WARN_QA} ${ERROR_QA}"
> @@ -421,6 +421,30 @@ def package_qa_check_desktop(path, name, d, elf, messages):
>          for l in output:
>              messages.append("Desktop file issue: " + l.strip())
>  
> +QAPATHTEST[textrel] = "package_qa_textrel"
> +def package_qa_textrel(path, name, d, elf, messages):
> +    """
> +    Check if the binary contains relocations in .text
> +    """
> +
> +    if not elf:
> +        return
> +
> +    if os.path.islink(path):
> +        return
> +
> +    phdrs = elf.run_objdump("-p", d)
> +    sane = True
> +
> +    import re
> +    textrel_re = re.compile("\s+TEXTREL\s+")
> +    for line in phdrs.split("\n"):
> +        if textrel_re.match(line):
> +	   sane = False
> +
> +    if not sane:
> +        messages.append("ELF binary '%s' has relocations in .text" % path)
> +
>  QAPATHTEST[ldflags] = "package_qa_hash_style"
>  def package_qa_hash_style(path, name, d, elf, messages):
>      """
> -- 
> 1.7.10.4
> 
> 
> 
> 
> _______________________________________________
> Openembedded-core mailing list
> Openembedded-core@lists.openembedded.org
> http://lists.linuxtogo.org/cgi-bin/mailman/listinfo/openembedded-core
Phil Blundell - Oct. 3, 2012, 10:44 a.m.
On Wed, 2012-10-03 at 12:31 +0200, Martin Jansa wrote:
> On Wed, Oct 03, 2012 at 11:24:12AM +0100, Phil Blundell wrote:
> > Signed-off-by: Phil Blundell <pb@pbcl.net>
> 
> Can you add a bit longer description of possible issues with relocations
> in .text? So that people seeing this issue will know how dangerous it is
> for them?
> 
> From my understanding (after reading
> http://www.gentoo.org/proj/en/hardened/pic-fix-guide.xml) it's mostly
> performance issue?

Yes, that's correct.  It basically falls into the same sort of category
as useless-rpaths; the binary will still work, but there will be some
adverse impact on performance and memory usage.  

Historically, the most common cause of DT_TEXTREL was accidentally
linking non-PIC code into a DSO.  Recent versions of the linker will
flatly refuse to do this on at least some architectures, though, so
hopefully this problem will just go away over time.

p.
Richard Purdie - Oct. 3, 2012, 11:19 a.m.
On Wed, 2012-10-03 at 11:44 +0100, Phil Blundell wrote:
> On Wed, 2012-10-03 at 12:31 +0200, Martin Jansa wrote:
> > On Wed, Oct 03, 2012 at 11:24:12AM +0100, Phil Blundell wrote:
> > > Signed-off-by: Phil Blundell <pb@pbcl.net>
> > 
> > Can you add a bit longer description of possible issues with relocations
> > in .text? So that people seeing this issue will know how dangerous it is
> > for them?
> > 
> > From my understanding (after reading
> > http://www.gentoo.org/proj/en/hardened/pic-fix-guide.xml) it's mostly
> > performance issue?
> 
> Yes, that's correct.  It basically falls into the same sort of category
> as useless-rpaths; the binary will still work, but there will be some
> adverse impact on performance and memory usage.  
> 
> Historically, the most common cause of DT_TEXTREL was accidentally
> linking non-PIC code into a DSO.  Recent versions of the linker will
> flatly refuse to do this on at least some architectures, though, so
> hopefully this problem will just go away over time.

Am I right in thinking this is also a marginal help to 'security' since
if the .text segment is loaded read only, it becomes slightly harder for
certain kinds of overflow attacks to work?

Cheers,

Richard
Phil Blundell - Oct. 3, 2012, 12:39 p.m.
On Wed, 2012-10-03 at 12:19 +0100, Richard Purdie wrote:
> Am I right in thinking this is also a marginal help to 'security' since
> if the .text segment is loaded read only, it becomes slightly harder for
> certain kinds of overflow attacks to work?

Possibly a marginal help, though (for glibc at least) the dynamic linker
will restore the original protection on .text once the relocations have
been applied, so the window of time during which you could mount an
attack based on the writeable .text region will be fairly small.  But in
principle you're right, for best security you don't want to have any
regions which are both writeable and executable.

p.

Patch

diff --git a/meta/classes/insane.bbclass b/meta/classes/insane.bbclass
index 5848ab4..ff35ed8 100644
--- a/meta/classes/insane.bbclass
+++ b/meta/classes/insane.bbclass
@@ -108,7 +108,7 @@  def package_qa_get_machine_dict():
 
 
 # Currently not being used by default "desktop"
-WARN_QA ?= "ldflags useless-rpaths rpaths unsafe-references-in-binaries unsafe-references-in-scripts staticdev libdir"
+WARN_QA ?= "ldflags useless-rpaths rpaths unsafe-references-in-binaries unsafe-references-in-scripts staticdev libdir textrel"
 ERROR_QA ?= "dev-so debug-deps dev-deps debug-files arch la2 pkgconfig la perms dep-cmp"
 
 ALL_QA = "${WARN_QA} ${ERROR_QA}"
@@ -421,6 +421,30 @@  def package_qa_check_desktop(path, name, d, elf, messages):
         for l in output:
             messages.append("Desktop file issue: " + l.strip())
 
+QAPATHTEST[textrel] = "package_qa_textrel"
+def package_qa_textrel(path, name, d, elf, messages):
+    """
+    Check if the binary contains relocations in .text
+    """
+
+    if not elf:
+        return
+
+    if os.path.islink(path):
+        return
+
+    phdrs = elf.run_objdump("-p", d)
+    sane = True
+
+    import re
+    textrel_re = re.compile("\s+TEXTREL\s+")
+    for line in phdrs.split("\n"):
+        if textrel_re.match(line):
+	   sane = False
+
+    if not sane:
+        messages.append("ELF binary '%s' has relocations in .text" % path)
+
 QAPATHTEST[ldflags] = "package_qa_hash_style"
 def package_qa_hash_style(path, name, d, elf, messages):
     """