New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.
Starred by 5 users

Issue metadata

Status: Fixed
Owner:
User never visited
Closed: Jan 2010
Components:



Sign in to add a comment

NaCl/x86 appears to leave return addresses unaligned when returning through the springboard.

Project Member Reported by cbiffle@google.com, Jan 13 2010 Back to list

Issue description

When returning from trusted to untrusted code, we must sanitize the return 
address before taking it.  This ensures that untrusted code cannot use the 
syscall interface to vector execution to an arbitrary address.  This role 
is entrusted to the function NaClSandboxAddr, in sel_ldr.h.  Unfortunately, 
since r572, this function has been a no-op on x86.


-- What happened?

During a routine refactoring, code that once read
  aligned_tramp_ret = tramp_ret & ~(nap->align_boundary - 1);

was changed to read
  return addr & ~(uintptr_t)((1 << nap->align_boundary) - 1);

Besides the variable renames (which were intentional and correct), a shift 
was introduced, treating nap->align_boundary as the log2 of bundle size.  

We didn't notice this because NaCl on x86 uses a 32-byte bundle size.  On 
x86 with gcc, (1 << 32) == 1.  (I believe the standard leaves this behavior 
undefined, but I'm rusty.)  Thus, the entire sandboxing sequence became a 
no-op.

This change had four listed reviewers and was explicitly LGTM'd by two.  
Nobody appears to have noticed the change.


-- Impact

There is a potential for untrusted code on 32-bit x86 to unalign its 
instruction stream by constructing a return address and making a syscall.  
This could subvert the validator.  A similar vulnerability may affect x86-
64.

ARM is not affected for historical reasons: the ARM implementation masks 
the untrusted return address using a different method.
 
Project Member

Comment 1 by cbiffle@google.com, Jan 13 2010

Status: Fixed
Fixed in r1313.

Comment 2 by hbridge@google.com, Jan 29 2010

Labels: -Priority-Critical Pri-0

Comment 3 by hbridge@google.com, Jan 29 2010

Labels: -OpSys-All OS-All

Sign in to add a comment