[Qemu-devel] [PATCH] Security house-cleaning

[Qemu-devel] [PATCH] Security house-cleaning

[Tim]

[-- Attachment #1: Type: text/plain, Size: 1083 bytes --]

After noticing the string format vulnerability the other day, I decided
to do a quick audit of calls to commonly mis-used string functions.  I
came across a few that looked a bit troublesome, in a security sense, so
I have updated them to their safer, length-checking counterparts.

I have to say, the core QEMU code is quite clean, and I feel that much
more confident in using it for honeypot projects later on. ;-)  The
biggest culprit in terms of potential overflows, was the slirp code.
There were some disturbing instances where strings were being pulled
directly from the command line and tossed into a fixed-length buffer
with no checks. =-X  I can't say that I understand at all how slirp
works, so I don't know if it is exploitable.

I will continue to develop this patch, as I monitor and test the newest
code on HEAD.  I do not currently use many parts of the codebase in my
testing though, so be sure to test the patch well before committing it,
particularly the slirp changes.

thanks,
tim

PS - a README is included for you Hetz, if you want to place it on your
site. 

[-- Attachment #2: security_20040616.tar.gz --]
[-- Type: application/octet-stream, Size: 2575 bytes --]

Re: [Qemu-devel] [PATCH] Security house-cleaning

[Gianni Tedesco]

[-- Attachment #1: Type: text/plain, Size: 1956 bytes --]

On Wed, 2004-06-16 at 21:38 -0700, Tim wrote:
> I have to say, the core QEMU code is quite clean, and I feel that much
> more confident in using it for honeypot projects later on. ;-)  The
> biggest culprit in terms of potential overflows, was the slirp code.
> There were some disturbing instances where strings were being pulled
> directly from the command line and tossed into a fixed-length buffer
> with no checks. =-X  I can't say that I understand at all how slirp
> works, so I don't know if it is exploitable.

Thats only worrisome from a security perspective if qemu was designed to
run SUID, which I doubt that it is... Of course it's a bug and needs
fixing though.

What would be more worrying is if there were overflows in the packet
processing allowing (possibly compromised) guest OS or remote machines
to take over qemu process by sending an exploit in a malformed packet.

The other possible vector I can think of is if there are exploits in
specific devices where you have things like audio processing going on
etc... Also when USB support exists, it could be possible to send
packets containing an overflow that could trip up a driver and execute
code, so that will need careful attention too.

Anyway, it's nice to see someone cares and is looking at the security of
qemu ;)

A quick note on the patch: where you are replacing strcpy() with
strncpy(), you are better to use snprintf(buf, sizeof(buf), "%s",
input); as that guarantees nul termination. It also allows you to easily
check if input was truncated, in some cases, silent truncation could be
a bug.

PS. Could you send README and patch as 2 attachments next time, dealing
with attachments, and archives is a PITA when u just want to skim a
patch :)

-- 
// Gianni Tedesco (gianni at scaramanga dot co dot uk)
lynx --source www.scaramanga.co.uk/scaramanga.asc | gpg --import
8646BE7D: 6D9F 2287 870E A2C9 8F60 3A3C 91B5 7669 8646 BE7D


[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 189 bytes --]

Re: [Qemu-devel] [PATCH] Security house-cleaning

[Renzo Davoli]

On Thu, Jun 17, 2004 at 04:07:20PM +0100, Gianni Tedesco wrote:
> Thats only worrisome from a security perspective if qemu was designed to
> run SUID, which I doubt that it is... Of course it's a bug and needs
> fixing though.

One of the main pros of Qemu (among the others) it that it has been
designed NOT to run SUID.
The only piece of code that need root access is tuntap networking.
This problem can be circunvented by:
- using sudo for tuntap
- using user net (a.k.a slirp)
- using vde.

renzo

Re: [Qemu-devel] [PATCH] Security house-cleaning

[Panagiotis Issaris]

[-- Attachment #1: Type: text/plain, Size: 1195 bytes --]

Hi Renzo,

Renzo Davoli wrote:

>On Thu, Jun 17, 2004 at 04:07:20PM +0100, Gianni Tedesco wrote:
>  
>
>>Thats only worrisome from a security perspective if qemu was designed to
>>run SUID, which I doubt that it is... Of course it's a bug and needs
>>fixing though.
>>    
>>
>
>One of the main pros of Qemu (among the others) it that it has been
>designed NOT to run SUID.
>The only piece of code that need root access is tuntap networking.
>This problem can be circunvented by:
>- using sudo for tuntap
>  
>
If you run it using sudo, you can limit the users who are allowed
to run it, but the process will still run as root, which will not
prevent exploits being able to run with root-permissions.

>- using user net (a.k.a slirp)
>- using vde.
>  
>

With friendly regards,
Takis

-- 
------------------------------------------------------------------------
Panagiotis Issaris
Katholieke Universiteit Leuven
Division Production Engineering,
Machine Design and Automation
Celestijnenlaan 300B              panagiotis.issaris@mech.kuleuven.ac.be
B-3001 Leuven Belgium                 http://www.mech.kuleuven.ac.be/pma
------------------------------------------------------------------------


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 256 bytes --]

Re: [Qemu-devel] [PATCH] Security house-cleaning

[Sebastien Bechet]

Le jeu 17/06/2004 à 17:14, Renzo Davoli a écrit :
> One of the main pros of Qemu (among the others) it that it has been
> designed NOT to run SUID.
It can be useful to have another account (not root) to run qemu with
suid bit set and protect big image disk with this account from users
errors (rm ...).

Bye.

-- 
Sebastien Bechet <s.bechet@av7.net>
av7.net

Re: [Qemu-devel] [PATCH] Security house-cleaning

[Tim]

> One of the main pros of Qemu (among the others) it that it has been
> designed NOT to run SUID.
> The only piece of code that need root access is tuntap networking.
> This problem can be circunvented by:
> - using sudo for tuntap
> - using user net (a.k.a slirp)
> - using vde.

Other future considerations: 
- PCI Proxy support (if it is ever offically supported)
    How will the host OS allow access by QEMU guest in this case?
- Other bus (USB, firewire, etc) direct access to real hardware


Not trying to be alarmist.  Just being conservative with code
quality/security.

tim

Re: [Qemu-devel] [PATCH] Security house-cleaning

[Sander Nagtegaal]

*nothing to do with security*
On thing wich would also be a good thing to add ( I'm not sure QEMU has it ) , 
is drag and drop from the guest OS to the emulator. If anyone of you are 
familiair with virtual PC then you'll know what I'll mean.

Op donderdag 17 juni 2004 6:37 pm, schreef Tim:
> > One of the main pros of Qemu (among the others) it that it has been
> > designed NOT to run SUID.
> > The only piece of code that need root access is tuntap networking.
> > This problem can be circunvented by:
> > - using sudo for tuntap
> > - using user net (a.k.a slirp)
> > - using vde.
>
> Other future considerations:
> - PCI Proxy support (if it is ever offically supported)
>     How will the host OS allow access by QEMU guest in this case?
> - Other bus (USB, firewire, etc) direct access to real hardware
>
>
> Not trying to be alarmist.  Just being conservative with code
> quality/security.
>
> tim
>
>
> _______________________________________________
> Qemu-devel mailing list
> Qemu-devel@nongnu.org
> http://lists.nongnu.org/mailman/listinfo/qemu-devel

Re: [Qemu-devel] [PATCH] Security house-cleaning

[Gianni Tedesco]

[-- Attachment #1: Type: text/plain, Size: 1628 bytes --]

On Thu, 2004-06-17 at 09:37 -0700, Tim wrote:
> > One of the main pros of Qemu (among the others) it that it has been
> > designed NOT to run SUID.
> > The only piece of code that need root access is tuntap networking.
> > This problem can be circunvented by:
> > - using sudo for tuntap
> > - using user net (a.k.a slirp)
> > - using vde.
> 
> Other future considerations: 
> - PCI Proxy support (if it is ever offically supported)
>     How will the host OS allow access by QEMU guest in this case?
> - Other bus (USB, firewire, etc) direct access to real hardware

well first thought is even you aren't root, you are still playing with
PCI devices, and due to the coarse grained control on some platforms
(read: x86) if you can access PCI devices, you can 0wn the entire system
(man 2 iopl). USB is packet based so can be secured reasonably.

The easiest approach is to acquire the resource as root and drop your
privileges...

FD passing across fork isn't really feasable because you need to parse
commandline to know what nodes to open with both PCI and USB.

Another approach is having a daemon which mediates access over a socket
(either mediating all access for security, or passing fds for
performance). The former approach would allow you to use devices in
remote machines so as to avoid locking up your qemu machine. It all
sounds quite nice, but I'm not sure we need yet another daemon though :)

-- 
// Gianni Tedesco (gianni at scaramanga dot co dot uk)
lynx --source www.scaramanga.co.uk/scaramanga.asc | gpg --import
8646BE7D: 6D9F 2287 870E A2C9 8F60 3A3C 91B5 7669 8646 BE7D

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 189 bytes --]

Re: [Qemu-devel] [PATCH] Security house-cleaning

[Renzo Davoli]

> Other future considerations: 
> - PCI Proxy support (if it is ever offically supported)
>     How will the host OS allow access by QEMU guest in this case?
> - Other bus (USB, firewire, etc) direct access to real hardware

The "safe" mode to do that is to define some kind of daemon running
as root managing tha physical card or port access and qemu joins the 
daemon to have the services.
The daemon can have config files for the sysadm to define each user
privileges while the virtual machine itself runs with user permissions.
The obvious problem in doing that is performance.

	renzo

Re: [Qemu-devel] [PATCH] Security house-cleaning

[Tim]

> Thats only worrisome from a security perspective if qemu was designed to
> run SUID, which I doubt that it is... Of course it's a bug and needs
> fixing though.

Yes, I agree on both points.  There is little I can offer to this
project right now, besides testing, and general code cleanup.  I know
almost nothing about hardware emulation, so just trying to help out with
what I know...

> What would be more worrying is if there were overflows in the packet
> processing allowing (possibly compromised) guest OS or remote machines
> to take over qemu process by sending an exploit in a malformed packet.

Agreed.  The slirp code in particular worries me in this respect.

> A quick note on the patch: where you are replacing strcpy() with
> strncpy(), you are better to use snprintf(buf, sizeof(buf), "%s",
> input); as that guarantees nul termination. It also allows you to easily
> check if input was truncated, in some cases, silent truncation could be
> a bug.

Ahh, good point.  However, if you specify a size one less than the size
of your buffer, I believe strncpy fills the rest of your buffer w/
nulls, doesn't it?  Or is that OS-specific?  So far, I have been keeping
my size in strncpy() one less than the buffer size (see the 256->255
change on one particular buffer, or instance).  However, this method is
prone to off-by-one bugs, so I might switch to snprintf() in some cases,
as you suggest.

> PS. Could you send README and patch as 2 attachments next time, dealing
> with attachments, and archives is a PITA when u just want to skim a
> patch :)

Sure thing, I'll do that next time.

Thanks for the input!
tim

Re: [Qemu-devel] [PATCH] Security house-cleaning

[Gianni Tedesco]

[-- Attachment #1: Type: text/plain, Size: 2154 bytes --]

On Thu, 2004-06-17 at 09:05 -0700, Tim wrote:
> > Thats only worrisome from a security perspective if qemu was designed to
> > run SUID, which I doubt that it is... Of course it's a bug and needs
> > fixing though.
> 
> Yes, I agree on both points.  There is little I can offer to this
> project right now, besides testing, and general code cleanup.  I know
> almost nothing about hardware emulation, so just trying to help out with
> what I know...

cool :)

> > What would be more worrying is if there were overflows in the packet
> > processing allowing (possibly compromised) guest OS or remote machines
> > to take over qemu process by sending an exploit in a malformed packet.
> 
> Agreed.  The slirp code in particular worries me in this respect.

I'll stick it on my todo list to have a look around in there when i'm
bored.

> > A quick note on the patch: where you are replacing strcpy() with
> > strncpy(), you are better to use snprintf(buf, sizeof(buf), "%s",
> > input); as that guarantees nul termination. It also allows you to easily
> > check if input was truncated, in some cases, silent truncation could be
> > a bug.
> 
> Ahh, good point.  However, if you specify a size one less than the size
> of your buffer, I believe strncpy fills the rest of your buffer w/
> nulls, doesn't it?  Or is that OS-specific?  So far, I have been keeping
> my size in strncpy() one less than the buffer size (see the 256->255
> change on one particular buffer, or instance).  However, this method is
> prone to off-by-one bugs, so I might switch to snprintf() in some cases,
> as you suggest.

nooooope, strcpy has no way of knowing how big the buffer is other than
what you tell it. It's likely that all (or most) of the buffers that are
strcpy'd to are initialised to zero / .bss so it doesn't matter in
reality, but better safe than sorry. What if some buffer is moved to
stack later, that would expose the latent bug.

-- 
// Gianni Tedesco (gianni at scaramanga dot co dot uk)
lynx --source www.scaramanga.co.uk/scaramanga.asc | gpg --import
8646BE7D: 6D9F 2287 870E A2C9 8F60 3A3C 91B5 7669 8646 BE7D

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 189 bytes --]

Re: [Qemu-devel] [PATCH] Security house-cleaning

[Tim]

> nooooope, strcpy has no way of knowing how big the buffer is other than
> what you tell it. It's likely that all (or most) of the buffers that are
> strcpy'd to are initialised to zero / .bss so it doesn't matter in
> reality, but better safe than sorry. What if some buffer is moved to
> stack later, that would expose the latent bug.

Oh... you are right.  It only pads with nulls if the src string is
shorter than the length argument.  I'll fix those and submit a new
version in a day or two.

thanks,
tim