Hi Pierre, * Pierre Chifflier <[EMAIL PROTECTED]> [2007-10-12 11:55]: > On Thu, Oct 11, 2007 at 01:27:17AM +0200, Nico Golde wrote: > > Version: 0.5.2-1.1sarge2 [...] > The login system has changed a lot since 0.5.2. At the first look, I > believe the exploit will not work for 0.5.2, or not the same way. > The real problem was caused by a memset with a wrong length, which was > introduced on recent versions (which means etch, testing and unstable > are impacted). Sarge version does not have this problem.
The sarge version *is* affected to the off-by-one. I fail to
see why this is not the real problem.
Look at src/wzd_ClientThread.c in the sarge version,
starting line 3270:
3246 char buffer[BUFFER_LEN];
....
3270 ret =
(context->read_fct)(context->controlfd,buffer,BUFFER_LEN,0,HARD_XFER_TIMEOUT,context);
3271
3272 if (ret == 0) {
3273 out_err(LEVEL_FLOOD,"Connection closed or timeout (socket
%d)\n",context->controlfd);
3274 return 1;
3275 }
3276 if (ret==-1) {
3277 out_err(LEVEL_FLOOD,"Error reading client response (socket
%d)\n",context->controlfd);
3278 return 1;
3279 }
3280
3281 /* this replace the memset (bzero ?) some lines before */
3282 buffer[ret] = '\0';
So what you do here is exactly the same you do in current version, you read
until BUFFER_LEN
bytes which is ok since sizeof(buffer) is BUFFER_LEN. The read function starts
filling the buffer
from element 0 so if you read BUFFER_LEN bytes or more you wrote until
buffer[BUFFER_LEN-1].
So far so good, the next you do is buffer[ret]='\0'. ret will be BUFFER_LEN in
the exploit scenario
so you write at buffer[BUFFER_LEN] and this *is* your off-by-one since your
overwrite the array bounds
here.
> The only fixable thing is a possible off-by-one in do_login_loop (patch
> attached).
>
> I'm also working on patches for other versions as well (feel free to NMU
> if you want).
[...]
> - ret =
> (context->read_fct)(context->controlfd,buffer,BUFFER_LEN,0,HARD_XFER_TIMEOUT,context);
> + ret =
> (context->read_fct)(context->controlfd,buffer,BUFFER_LEN-1,0,HARD_XFER_TIMEOUT,context);
This would work but would not be a correct patch. This would be the same like
adding one element
to your buffer size but would not solve the real problem. You don't want just
to read BUFFER_LEN -1,
why should you, uour buffer has BUFFER_LEN elements. What you want to do is
writing the null to
buffer[ret - 1] since thats where the last element is if you read BUFFER_LEN
(see my patch).
Uploading my NMU with your permission now.
Kind regards
Nico
--
Nico Golde - http://ngolde.de - [EMAIL PROTECTED] - GPG: 0x73647CFF
For security reasons, all text in this mail is double-rot13 encrypted.
pgpMtbO5RviQe.pgp
Description: PGP signature
