5:45 a.m.
I've updated the patch. It's now in regressions, and checks both libguestfs.so
and the guest daemon. It occurs to me that it's the daemon that's would get an
executable stack from the specific patch I posted the other day. However, would
this actually matter specifically for the daemon?
5:45 a.m.
---
regressions/Makefile.am | 4 ++-
regressions/test-noexec-stack.pl | 63 ++++++++++++++++++++++++++++++++++++++
2 files changed, 66 insertions(+), 1 deletions(-)
create mode 100755 regressions/test-noexec-stack.pl
diff --git a/regressions/Makefile.am b/regressions/Makefile.am
index 12c4e80..555efd4 100644
--- a/regressions/Makefile.am
+++ b/regressions/Makefile.am
@@ -22,6 +22,7 @@
# See also capitests/
TESTS = \
+ test-noexec-stack.pl \
rhbz503169c10.sh \
rhbz503169c13.sh \
test-cancellation-upload-daemoncancels.sh \
@@ -41,7 +42,8 @@ FAILING_TESTS = \
TESTS_ENVIRONMENT = \
LD_LIBRARY_PATH=$(top_builddir)/src/.libs \
- LIBGUESTFS_PATH=$(top_builddir)/appliance
+ LIBGUESTFS_PATH=$(top_builddir)/appliance \
+ NOEXEC_CHECK="$(top_builddir)/src/.libs/libguestfs.so
$(top_builddir)/daemon/guestfsd"
EXTRA_DIST = \
$(FAILING_TESTS) \
diff --git a/regressions/test-noexec-stack.pl b/regressions/test-noexec-stack.pl
new file mode 100755
index 0000000..9703066
--- /dev/null
+++ b/regressions/test-noexec-stack.pl
@@ -0,0 +1,63 @@
+#!/usr/bin/perl
+
+use strict;
+use warnings;
+
+die("NOEXEC_CHECK not set") unless(exists($ENV{NOEXEC_CHECK}));
+
+my @files = split(/ /, $ENV{NOEXEC_CHECK});
+
+FILES: foreach my $file (@files) {
+ my $output;
+ open($output, '-|', "readelf -l $file")
+ or die("\"readelf -l $file\" failed");
+
+ my $offset;
+ my $line = 1;
+
+ # Find the offset of the Flags field
+ while(<$output>) {
+ next unless(/^\s*Type\b/);
+
+ my @lines;
+ push(@lines, $_);
+
+ # Look for a Flg field on this line (32 bit)
+ $offset = index($_, 'Flg ');
+
+ if(-1 == $offset) {
+ # 64 bit is split over 2 lines. Look for a Flags field on the next
+ # line
+ $_ = <$output>;
+ $offset = index($_, 'Flags ');
+ $line = 2;
+ push(@lines, $_);
+ }
+
+ die("Unrecognised header: ".join("\n", @lines)) if(-1 ==
$offset);
+ last;
+ }
+
+ # Find the GNU_STACK entry
+ while(<$output>) {
+ next unless(/^\s*GNU_STACK\b/);
+
+ # Skip over input lines according to the header
+ for(my $i = 1; $i < $line; $i++) {
+ $_ = <$output>;
+ }
+
+ my $flags = substr($_, $offset, 3);
+
+ $flags =~ /^[ R][ W]([ E])$/ or die("Unrecognised flags: $flags");
+
+ if('E' eq $1) {
+ print "***** $ARGV[0] has an executable stack *****\n";
+ exit(1);
+ }
+
+ next FILES;
+ }
+
+ die("Didn't find GNU_STACK entry");
+}
--
1.6.2.5
7:16 a.m.
Matthew Booth wrote:
---
regressions/Makefile.am | 4 ++-
...
diff --git a/regressions/test-noexec-stack.pl
b/regressions/test-noexec-stack.pl
...
+FILES: foreach my $file (@files) {
+ my $output;
+ open($output, '-|', "readelf -l $file")
+ or die("\"readelf -l $file\" failed");
Hi Matt,
A general tip:
There's a bit of a risk in opening a pipe like that.
Imagine that $file contains certain shell meta-characters (like "; rm -rf /").
Use an array, and you avoid that, as well as the duplication in the "die":
my @cmd = ('readelf', '-l', $file);
open($output, '-|', @cmd)
or die "$0: failed to run: \`" . join(' ',@cmd) . "':
$!\n";
Also, imho, every diagnostic should start with "$program_name: ".
That makes it easier to know which program produced a particular
string, when wading through thousands of lines of output from
hundreds of different tools.
7:40 a.m.
On Wed, Aug 05, 2009 at 11:45:03AM +0100, Matthew Booth wrote:
---
regressions/Makefile.am | 4 ++-
regressions/test-noexec-stack.pl | 63 ++++++++++++++++++++++++++++++++++++++
2 files changed, 66 insertions(+), 1 deletions(-)
create mode 100755 regressions/test-noexec-stack.pl
diff --git a/regressions/Makefile.am b/regressions/Makefile.am
index 12c4e80..555efd4 100644
--- a/regressions/Makefile.am
+++ b/regressions/Makefile.am
@@ -22,6 +22,7 @@
# See also capitests/
TESTS = \
+ test-noexec-stack.pl \
rhbz503169c10.sh \
rhbz503169c13.sh \
Keep these in alphabetical order, and what Jim said. Otherwise
the patch looks fine.
Rich.
--
Richard Jones, Emerging Technologies, Red Hat http://et.redhat.com/~rjones
New in Fedora 11: Fedora Windows cross-compiler. Compile Windows
programs, test, and build Windows installers. Over 70 libraries supprt'd
http://fedoraproject.org/wiki/MinGW http://www.annexia.org/fedora_mingw
5:58 a.m.
On Wed, Aug 05, 2009 at 11:45:02AM +0100, Matthew Booth wrote:
I've updated the patch. It's now in regressions, and checks
both
libguestfs.so and the guest daemon. It occurs to me that it's the
daemon that's would get an executable stack from the specific patch
I posted the other day. However, would this actually matter
specifically for the daemon?
Duh yes, you're right. I was checking the wrong executable.
$ readelf -l daemon/guestfsd
[...]
GNU_STACK 0x0000000000000000 0x0000000000000000 0x0000000000000000
0x0000000000000000 0x0000000000000000 RWE 8
So your patch *does* make the daemon stack executable.
Does it matter for the daemon? Probably yes. We should avoid
allowing code to be injected into the appliance. For example if there
was a malicious string which was passed into libguestfs from user
input, then the additional stack guards would be our last line of
defence.
Also rpmlint will give a warning about this.
NAK I'm afraid. However just hoisting those functions out so they are
not nested should make it OK. Don't forget the other two things I
raised in the original review.
Rich.
--
Richard Jones, Emerging Technologies, Red Hat http://et.redhat.com/~rjones
virt-top is 'top' for virtual machines. Tiny program with many
powerful monitoring features, net stats, disk stats, logging, etc.
http://et.redhat.com/~rjones/virt-top
6:01 a.m.
New subject: [PATCH] Add a test for an executable stack in libguestfs.so
On 05/08/09 11:58, Richard W.M. Jones wrote:
On Wed, Aug 05, 2009 at 11:45:02AM +0100, Matthew Booth wrote:
> I've updated the patch. It's now in regressions, and checks both
> libguestfs.so and the guest daemon. It occurs to me that it's the
> daemon that's would get an executable stack from the specific patch
> I posted the other day. However, would this actually matter
> specifically for the daemon?
Duh yes, you're right. I was checking the wrong executable.
$ readelf -l daemon/guestfsd
[...]
GNU_STACK 0x0000000000000000 0x0000000000000000 0x0000000000000000
0x0000000000000000 0x0000000000000000 RWE 8
So your patch *does* make the daemon stack executable.
Does it matter for the daemon? Probably yes. We should avoid
allowing code to be injected into the appliance. For example if there
was a malicious string which was passed into libguestfs from user
input, then the additional stack guards would be our last line of
defence.
Also rpmlint will give a warning about this.
NAK I'm afraid. However just hoisting those functions out so they are
not nested should make it OK. Don't forget the other two things I
raised in the original review.
Just to confirm, that NAK is for the original patch, not for this patch?
Agree I'll update the original patch not to use nested function.
Matt
--
Matthew Booth, RHCA, RHCSS
Red Hat Engineering, Virtualisation Team
M: +44 (0)7977 267231
GPG ID: D33C3490
GPG FPR: 3733 612D 2D05 5458 8A8A 1600 3441 EA19 D33C 3490
7:39 a.m.
On Wed, Aug 05, 2009 at 12:01:19PM +0100, Matthew Booth wrote:
Just to confirm, that NAK is for the original patch, not for this
patch?
Yes, NAK to the original patch which put nested functions in
devsparts.c.
Rich.
--
Richard Jones, Emerging Technologies, Red Hat http://et.redhat.com/~rjones
virt-top is 'top' for virtual machines. Tiny program with many
powerful monitoring features, net stats, disk stats, logging, etc.
http://et.redhat.com/~rjones/virt-top
2:58 a.m.
I added a copyright and license (GPL) notice to that file:
http://git.et.redhat.com/?p=libguestfs.git;a=commitdiff;h=65326fe130cbb08...
Every source file needs to have one.
Rich.
--
Richard Jones, Emerging Technologies, Red Hat http://et.redhat.com/~rjones
Read my programming blog: http://rwmj.wordpress.com
Fedora now supports 75 OCaml packages (the OPEN alternative to F#)
http://cocan.org/getting_started_with_ocaml_on_red_hat_and_fedora
5511
days inactive
5512
days old
7 comments
3 participants
participants (3)
-
Jim Meyering -
Matthew Booth -
Richard W.M. Jones