[Bug libc/2499] New: dns_getcanonname() unaligned problems

[Bug libc/2499] New: dns_getcanonname() unaligned problems

[david dot mckay at st dot com]

While working on a glibc port to another processor, I came across a couple of       
possible problems with the dns_getcanonname() function. The first is in      
__lib_res_nquery() in resolve/res_query.c. The problem is that the     
libc_res_nsend() function can reallocate the answer buffer. However on return     
from this function only the original buffer is checked, not the buffer where     
the answer data actually is. This results in dereferencing a uninitialised     
stack location at the ntohs(hp->ancount) statement, and looking at the wrong    
rcode return value.  
    
The second problem is in the  _nss_dns_getcanonname_r() function in    
resolv/nss_dns/dns-canon.c. Here it assumes 16 bit alignment of data, whereas   
the data is not necessarily aligned to this boundary, resulting in a misaligned   
fault.   
   
These possible bugs are present in 2.3.3 and appears to be present in the   
latest CVS sources. Though I may be completely wrong in my analysis of course!   
 
It may also be desirable to initialise these fields in res_nsend() when the 
buffer is realloced, but my patch doesn't do this as I am not sure it is 
needed.  
 
Patch:   
   
--- libc/resolv/res_query.c.orig        2006-03-30 17:24:00.000000000 +0100   
+++ libc/resolv/res_query.c     2006-03-31 13:24:59.000000000 +0100   
@@ -163,6 +163,8 @@   
                return (n);   
        }   
   
+       hp = (HEADER*) (*answerp);   
+   
        if (hp->rcode != NOERROR || ntohs(hp->ancount) == 0) {   
 #ifdef DEBUG   
                if (statp->options & RES_DEBUG)   
--- libc/resolv/nss_dns/dns-canon.c.orig        2006-03-31 17:47:39.000000000   
+0100   
+++ libc/resolv/nss_dns/dns-canon.c     2006-03-31 17:55:05.000000000 +0100   
@@ -101,7 +101,7 @@   
              ptr += s;   
   
              /* Check whether type and class match.  */   
-             unsigned int type = ntohs (*(uint16_t *) ptr);   
+             unsigned int type = ntohs (ns_get16(ptr));   
              if (type == qtypes[i])   
                {   
                  /* We found the record.  */   
@@ -131,14 +131,14 @@   
                goto unavail;   
   
              ptr += sizeof (uint16_t);   
-             if (*(uint16_t *) ptr != htons (ns_c_in))   
+             if (ns_get16(ptr) != htons (ns_c_in))   
                goto unavail;   
   
              /* Also skip over the TTL.  */   
              ptr += sizeof (uint16_t) + sizeof (uint32_t);   
   
              /* Skip over the data length and data.  */   
-             ptr += sizeof (uint16_t) + ntohs (*(uint16_t *) ptr);   
+             ptr += sizeof (uint16_t) + ntohs (ns_get16(ptr));   
            }   
        }   
     }

-- 
           Summary: dns_getcanonname() unaligned problems
           Product: glibc
           Version: 2.3.3
            Status: NEW
          Severity: normal
          Priority: P2
         Component: libc
        AssignedTo: drepper at redhat dot com
        ReportedBy: david dot mckay at st dot com
                CC: glibc-bugs at sources dot redhat dot com
 GCC build triplet: i686-pc-linux-gnu
  GCC host triplet: st231-linux
GCC target triplet: st231-linux


http://sourceware.org/bugzilla/show_bug.cgi?id=2499

------- You are receiving this mail because: -------
You are on the CC list for the bug, or are watching someone who is.

[Bug libc/2499] dns_getcanonname() unaligned problems

[david dot mckay at st dot com]

------- Additional Comments From david dot mckay at st dot com  2006-04-03 10:17 -------
*** Bug 2504 has been marked as a duplicate of this bug. ***

-- 


http://sourceware.org/bugzilla/show_bug.cgi?id=2499

------- You are receiving this mail because: -------
You are on the CC list for the bug, or are watching someone who is.

[Bug libc/2499] dns_getcanonname() unaligned problems

[drepper at redhat dot com]

------- Additional Comments From drepper at redhat dot com  2006-05-06 19:20 -------
While the place you indicated indeed have problems, all the changes you proposed
are wrong.  Amazing.  It also shows you never tested the changes.

What interests me is how you came across the res_query.c problems.  In all the
years that we have this code I cannot remember seeing any indication that this
was ever a problem.

I checked in correct changes upstream.

-- 
           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|NEW                         |RESOLVED
         Resolution|                            |FIXED


http://sourceware.org/bugzilla/show_bug.cgi?id=2499

------- You are receiving this mail because: -------
You are on the CC list for the bug, or are watching someone who is.

[Bug libc/2499] dns_getcanonname() unaligned problems

[david dot mckay at st dot com]

------- Additional Comments From david dot mckay at st dot com  2006-05-08 14:03 -------
(In reply to comment #2)  
> While the place you indicated indeed have problems, all the changes you   
> proposed  
> are wrong.  Amazing.  It also shows you never tested the changes.  
  
Yes, you are quite right, the patch is wrong. I misunderstood what ns_get16()  
does. Apologies. 
  
I did run it, and it appeared to solve the problem I had. Probably worked for 
the same reason the original code worked even though it was looking at the 
wrong data. 
 
> What interests me is how you came across the res_query.c problems.  In all   
> the  
> years that we have this code I cannot remember seeing any indication that   
> this  
> was ever a problem.  
  
Yes, I was amazed that these bug have not been spotted before. I came across 
them quite easily. It was telnetd that caused the problem. Basically, I could 
not telnet in as it hit these bugs. The nasty thing was that on another machine  
running identical software I could. After some head scratching I realised the 
difference was the length of the target hostname, one had an odd number of 
chars and the other even. Even failed. 
 
The instruction trace from the simulator clearly showed up both  
problems easily.  
 
I looked at what happend on a superh, and that machine went through the error 
case as ntohs(hp->ancount)==0 happened to be true due to the stack layout, 
whereas the ST200 didn't. I suspect the stack may always be in a defined state 
when this function is called, and luck has dictated that it goes through the 
error case on most machines, which is why you would not have seen the resulting 
misaligned reported before. 
 
The unaligned accesses would not be seen on x86 of course, and would be fixed 
up by the kernel anyway on most arches. I chose to kill unaligned user accesses 
instead of fix them up for the ST200, which is why I saw it so obviously. 
  
A strange combination of problems I think. It would be interesting to figure 
out exactly what is going on on x86. 
 
The only vague hint that someone else may have seen this that I could find is: 
 
http://www.redhat.com/archives/fedora-desktop-list/2004-September/msg00031.html 
 
Where valgrind detects a conditional jump or move dependant on uninitialised 
value in the res_nquery() function. 
 
> I checked in correct changes upstream.  
 
Thanks. At least my analysis was right even if the patch wasn't:-) 
 

-- 


http://sourceware.org/bugzilla/show_bug.cgi?id=2499

------- You are receiving this mail because: -------
You are on the CC list for the bug, or are watching someone who is.