Re: [Xen-devel] Critique of the Xen Security Process

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 06.11.15 at 18:22, <joanna@xxxxxxxxxxxxxxxxxxxxxx> wrote:
> 1. First of all, I wish Xen was somehow more defensively coded. To provide 
> some
> examples:
> 
> a. In XSA-109 [5] there was a problem with the hypervisor dereferencing a 
> NULL
> pointer. The problem was fixed by the Xen Security Team by applying a patch
> which (hopefully) made sure the execution path that lead to this NULL 
> pointer
> dereferencing code was never taken. Back then I suggested (on the Xen
> pre-disclosure list) to make this patch more explicit though:
> 
>> On Wed, Jan 21, 2015 at 02:31:51PM +0100, Joanna Rutkowska wrote:            
>     
>> (...)
>>>                                                                             
>>>  
>>> Wouldn't it be prudent to also check if:                                    
>>>  
>>>                                                                             
>>>  
>>> (v->arch.paging.mode>{write_guest_entry,cmpxchg_guest_entry} != NULL)       
>>>    
>>>                                                                             
>>>  
>>> ... in the two affected functions, just before derefing these function      
>>>  
>>> pointers?                                                                   
>>>  
>>>                                                                             
>>>  
>>> Going even a step further: how about replacing all                          
>>>  
>>> function-pointer-based calls with macros that first validates the           
>>>  
>>> pointer before derefing it? At least when the system doesn't have SMEP?     
>>>  
>>>                                                                             
>>>  
> ...to which I got a reply from one of the Xen Security Team engineers that 
> the
> above might perhaps be justified in debug builds only, followed by a 
> standard:
> "feel free to contribute a patch".

And you know what - I can only repeat this now. Indeed, the points
you make are all quite reasonable from a technical perspective (and
especially the code quality aspect is something that I also have been
complaining _but also trying to do something_ about repeatedly), but
the attitude behind the way you present them bothers me quite a lot:
I get the impression that you (a) expect others to do work that you
neither pay for, nor are at the slightest willing to help with and (b)
de-value all our efforts during the last so many years to move the
code base from a research project to a production usable one.

Yes, the work item you mention above has been on our virtual,
combined todo list, but with all the other work that needs to be done
it seems like no-one actually found the time to actually carry this out.
A good example that just pointing out issues is nice and easy, but
actually finding time to do the work is quite a bit harder.

> b. The XSA-123 [6] was another critical security bug in Xen, this time 
> resulting
> from one of the hypervisor developer's fetish to use an absolutely confusing
> construct in order to save a few modest bytes in a structure which might 
> have
> been allocated by the system maybe a few tens of times at best. Even more
> worrying was the way how Xen Security Team decided to fix the bug: again by
> modifying some condition in the code further up the execution path, with the
> hopes that this time they would ensure this puzzling construct would always 
> be
> used properly. We wrote more about this in our QSB #18 [7].

I suppose you already know what my response here would be?

> c. Finally, the way how Xen fixed the recent XSA-148 looks also very reactive,
> IMHO. With a bug of this calibre, I would expect Xen to carefully review and
> augment all its PV memory virtualization code with additional checks 
> (ASSERTs),
> ensuring certain invariants are always satisfied. Such as e.g. that none of 
> the
> pages containing PDEs or PTEs are becoming writeable by the VM.

Perhaps I'm overlooking something, but I cannot see what kinds of
ASSERT()s could have helped avoid that problem. The only things I
can see that would have helped here are
- better thinking through of all cases by the original developer
- better review before things going in
- regular code audits
None of which, according to what you write further down, you're
willing to help with.

> I can't help but have a feeling that some of the Xen developers seem to be
> overconfident in their belief they can fully understand all the possible
> execution paths in their code. Well, the XSAs quoted above are an 
> indisputable
> prove that this is not quite always the case. Realizing that, each developer 
> by
> themselves, might be a great step towards a more secure hypervisor...

I think we need to find a reasonable balance here. Taking your
statement to the extreme, we might end up with code having more
ASSERT()s in it than anything else. Surely that can't be the goal,
as this would severely hamper maintainability. It simply doesn't
make sense for every function to mistrust its callers in all aspects.

> 2. Another security-related aspect of the Xen project is how it totally 
> ignores
> problems related to the build process security. Those who don't believe me
> should grep the sources for wget, which is now disguised as "FETCHER" shell
> variable... (so grep for "FETCHER" string)
> 
> I feel embarrassed that I need to explain, at the end of 2015, why the build
> process of any serious software project should not blindly download unsigned
> components (sources) from the Internet, especially if it is about to execute
> Makefiles from these components a moment later... Come on, guys!
> 
> (Of course we have been forced to get around this gapping security whole in
> Qubes OS [8] ourselves, sadly with a method that is not well suited for
> upstreaming).

To be honest, for a purely development oriented build environment
this might by fine. No-one is required to use that, or even run their
builds outside of sandboxes. Even in my local build environment
(used for just my own purposes) I avoid any such downloads, albeit
perhaps more for performance than security reasons. But what this
tells us is that avoiding to build on this "convenience" is quite simple.

> 3. Another thing is, of course: stop adding features to the core hypervisor. 
> We
> really need Xen to finally mature, stabilize, and for its development process 
> to
> be slowing down over time (just the bug fixes). We need a long-term-supported
> hypervisor, which doesn't change with subsonic speed. This would allow this 
> core
> code to be widely audited by many experts. If some users want features, these
> should perhaps be maintained as additional modules (no, I don't mean 
> dynamically
> loaded modules, just compile-time included), preferably in separate repos.

I admit that, especially during the last couple of months, I have
wished the ratio between feature addition patches and bug fixes
would shift towards the latter (and that without hoping for there
to be more bugs to be fixed - in fact the opposite has seemed to
be the case in the more recent past), albeit for other reasons than
yours.

But is not adding new features realistic? I don't think so. People
want a secure _and_ functionally complete _and_ performant
hypervisor, and the latter two require making use of new
hardware features as they become available (and occasionally
introduce pure software features too, see the xSplice work
underway).

> Admittedly, some of the features are a result of hardware evolution, such as
> e.g. UEFI support. But many are not. Again, maintaining these as optional code
> (in separate repos) would be a great step into getting the hypervisor 
> maturing,
> finally.

This, afaict, would quickly become unwieldy and unmaintainable.

> 4. Finally, I've been really surprised by the line of reasoning Ian 
> expressed in
> the above-mentioned blog post. TL;DR: "we're still doing pretty great, 
> compared
> to other projects, because: 1) we have smaller number of publicly disclosed
> bugs, and 2) we actually publicly disclose these bugs which we are aware 
> of".
> 
> The attitude presented in the blog post is so wrong, that I'm not even sure
> where to start commenting on this...
> 
> With a single bug like the XSA-148 which, let me repeat that one more time: 
> had
> been present in the hypervisor for the last 7 years, so with a bug like this 
> it
> really doesn't matter how many (i.e. what number) of critical bugs does the
> competition have.

And let me repeat - it doesn't matter for how long the bug has been
there without anyone noticing. Had you noticed it 7 years ago, and
had we both rejected your fixes and not taken steps to fix it
ourselves, I could see you being upset. But you having done nothing
(Out of the now around 150 security issues in Xen, how many did you
discover, btw?), what gives you the right to complain about a bug (no
matter how severe) having been there for an admittedly rather long
time? If you weren't able to spot it, how can you demand someone
else to be able to? It is the nature of bugs that at times they hide
rather well.

> Can we, the Qubes OS project, or myself personally, help with implementing the
> above suggestions? Sadly, no. While some of us do contribute occasional 
> patches
> to Xen (specifically Marek Marczykowski-GÃrecki), we really work for a 
> different
> project and have different tasks and responsibilities.

But you seem to forget that many if not most of us too are not
employed by their employers to look after Xen as their sole or
primary task. I.e. if everyone reasoned this way, we could simply
bin the entire project.

So bottom line - _constructive_ criticism is welcome, but
accusations, especially without any intent to help changing things
(other than demanding others to do the necessary work), aren't.

(Please note that all of the above are my personal views, i.e. I'm
explicitly not speaking on behalf of the project.)

Jan

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel