Re: [Ganglia-developers] patches for: [Sec] Gmetad server BoFandnetwork overload + [Feature] multiple requests per connoninteractive port
>>> On 1/15/2009 at 4:25 PM, in message , "Spike Spiegel" wrote: > On Fri, Jan 16, 2009 at 7:04 AM, Kostas Georgiou > wrote: >> On Thu, Jan 15, 2009 at 01:41:53PM -0700, Brad Nicholes wrote: >> >>> >>> On 1/15/2009 at 8:56 AM, in message >>> <496efa2a02ac0003a...@lucius.provo.novell.com>, "Brad Nicholes" >>> wrote: >>> >>> After taking a little closer look at the patch, I think we are OK as >>> far as the recursive call to process_path() is concerned since this >>> case is an error condition and should stop processing rather than >>> continuing in the recursive loop. > > indeed, this should work just fine. > >>> The other two concerns are still >>> there however. I still think that we are off-by-one in the malloc >>> call. It should be len+1 and I still think that we should limit the >>> malloc to 256 rather than allowing it to be unlimited. >> >> I agree about the off-by-one > > argh, my bad sorry, double dumb since I even considered the case. > len+1 it is and the comment should go, thanks. > >> but I am not too worried about a malloc >> limit, from what I can tell it can only get as high as REQUESTLEN. > > I agree with Kostas, as I wrote in my initial email I didn't worry > about that because of the REQUESTLEN boundary which is enforced in > readline. > Since it is limited by REQUESTLEN, I am OK with it. So it sounds like we just need a fix for the off-by-one and the check for NULL on the malloc. Brad -- This SF.net email is sponsored by: SourcForge Community SourceForge wants to tell your story. http://p.sf.net/sfu/sf-spreadtheword ___ Ganglia-developers mailing list Ganglia-developers@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/ganglia-developers
Re: [Ganglia-developers] patches for: [Sec] Gmetad server BoF andnetwork overload + [Feature] multiple requests per conn oninteractive port
On Fri, Jan 16, 2009 at 7:04 AM, Kostas Georgiou wrote: > On Thu, Jan 15, 2009 at 01:41:53PM -0700, Brad Nicholes wrote: > >> >>> On 1/15/2009 at 8:56 AM, in message >> <496efa2a02ac0003a...@lucius.provo.novell.com>, "Brad Nicholes" >> wrote: >> >> After taking a little closer look at the patch, I think we are OK as >> far as the recursive call to process_path() is concerned since this >> case is an error condition and should stop processing rather than >> continuing in the recursive loop. indeed, this should work just fine. >> The other two concerns are still >> there however. I still think that we are off-by-one in the malloc >> call. It should be len+1 and I still think that we should limit the >> malloc to 256 rather than allowing it to be unlimited. > > I agree about the off-by-one argh, my bad sorry, double dumb since I even considered the case. len+1 it is and the comment should go, thanks. > but I am not too worried about a malloc > limit, from what I can tell it can only get as high as REQUESTLEN. I agree with Kostas, as I wrote in my initial email I didn't worry about that because of the REQUESTLEN boundary which is enforced in readline. as to limiting the path to 256 I actually did that in my first implementation, but eventually converted to a malloc solution because I was reminded that "640 KB ought to be enough for everybody" and I could see no downsides. > > The malloc call needs to be checked for NULL and the comment that > "The recursive structure doesn't require any memory allocations" is > false now if malloc replaces the stack allocation. correct thanks everybody -- This SF.net email is sponsored by: SourcForge Community SourceForge wants to tell your story. http://p.sf.net/sfu/sf-spreadtheword ___ Ganglia-developers mailing list Ganglia-developers@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/ganglia-developers
Re: [Ganglia-developers] patches for: [Sec] Gmetad server BoF andnetwork overload + [Feature] multiple requests per conn oninteractive port
On Thu, Jan 15, 2009 at 01:41:53PM -0700, Brad Nicholes wrote: > >>> On 1/15/2009 at 8:56 AM, in message > <496efa2a02ac0003a...@lucius.provo.novell.com>, "Brad Nicholes" > wrote: > > After taking a little closer look at the patch, I think we are OK as > far as the recursive call to process_path() is concerned since this > case is an error condition and should stop processing rather than > continuing in the recursive loop. The other two concerns are still > there however. I still think that we are off-by-one in the malloc > call. It should be len+1 and I still think that we should limit the > malloc to 256 rather than allowing it to be unlimited. I agree about the off-by-one but I am not too worried about a malloc limit, from what I can tell it can only get as high as REQUESTLEN. The malloc call needs to be checked for NULL and the comment that "The recursive structure doesn't require any memory allocations" is false now if malloc replaces the stack allocation. Kostas -- This SF.net email is sponsored by: SourcForge Community SourceForge wants to tell your story. http://p.sf.net/sfu/sf-spreadtheword ___ Ganglia-developers mailing list Ganglia-developers@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/ganglia-developers
Re: [Ganglia-developers] patches for: [Sec] Gmetad server BoF andnetwork overload + [Feature] multiple requests per conn oninteractive port
>>> On 1/15/2009 at 8:56 AM, in message <496efa2a02ac0003a...@lucius.provo.novell.com>, "Brad Nicholes" wrote: On 1/14/2009 at 3:10 PM, in message > , "Jesse Becker" > wrote: >> On Wed, Jan 14, 2009 at 12:56, Brad Nicholes wrote: >>> Sounds good, can we condense the description below into something that we >> can put into the 3.1 STATUS file so that we can start the process of getting > >> this backported into 3.1? >> >> Done. Committed as r1947. >> >> I suggest that once this is accepted, we release 3.1.2 ASAP. >> >> Also, a big thanks to Spike for finding and tracking this problem down. > > > Spike, > Correct me if I am wrong, but I think we still have a one-off problem > with the patch. > > len = q-p; > /* +1 not needed as q-p is already accounting for that */ > element = malloc(len); > strncpy(element, p, len); > element[len] = '\0'; > > > In the above code suppose "len" is 10 so we malloc() 10 bytes which means > that the array indexing for element is 0-9. Then we strncpy() 10 bytes into > "element" which is OK. However setting element[len] = "\0", IOW element[10] > = "\0" is indexing into the element array 1 byte beyond the actual length of > element. So either the code needs to be element = malloc(len+1) or > element[len-1] = "\0" correct? > > I would suggest that maybe rather than allowing the code to malloc() any > size buffer to match the incoming element length that instead or in addition > we limit the element length as well. Allowing the code to malloc() an > unlimited buffer can also lead to security issues. Since the original code > defined a 256 byte buffer, 256 seems to be a good limit for elements of the > path. It might be a better idea to simply add the limit checking rather than > doing a lot of malloc and free calls. If an element is larger than the > limit, then produce a mal-formed error message and bail out. > > Also I don't think that the current patch will work without the multi-path > patch that was also submitted, without being reworked. The current patch > removes the recursive call to process_path() which means that without the for > loop that was provided in the multi-path patch, only one element of the path > will be processed and the rest will be ignored. The current patch probably > needs to be reworked and separated from the multi-path patch. > After taking a little closer look at the patch, I think we are OK as far as the recursive call to process_path() is concerned since this case is an error condition and should stop processing rather than continuing in the recursive loop. The other two concerns are still there however. I still think that we are off-by-one in the malloc call. It should be len+1 and I still think that we should limit the malloc to 256 rather than allowing it to be unlimited. Brad -- This SF.net email is sponsored by: SourcForge Community SourceForge wants to tell your story. http://p.sf.net/sfu/sf-spreadtheword ___ Ganglia-developers mailing list Ganglia-developers@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/ganglia-developers
Re: [Ganglia-developers] patches for: [Sec] Gmetad server BoF andnetwork overload + [Feature] multiple requests per conn oninteractive port
On Thu, Jan 15, 2009 at 10:56, Brad Nicholes wrote: On 1/14/2009 at 3:10 PM, in message > , "Jesse Becker" > wrote: >> On Wed, Jan 14, 2009 at 12:56, Brad Nicholes wrote: >>> Sounds good, can we condense the description below into something that we >> can put into the 3.1 STATUS file so that we can start the process of getting >> this backported into 3.1? >> >> Done. Committed as r1947. >> >> I suggest that once this is accepted, we release 3.1.2 ASAP. >> >> Also, a big thanks to Spike for finding and tracking this problem down. > > > Spike, >Correct me if I am wrong, but I think we still have a one-off problem with > the patch. > > len = q-p; > /* +1 not needed as q-p is already accounting for that */ > element = malloc(len); > strncpy(element, p, len); > element[len] = '\0'; > > > In the above code suppose "len" is 10 so we malloc() 10 bytes which means > that the array indexing for element is 0-9. Then we strncpy() 10 bytes into > "element" which is OK. However setting element[len] = "\0", IOW element[10] > = "\0" is indexing into the element array 1 byte beyond the actual length of > element. So either the code needs to be element = malloc(len+1) or > element[len-1] = "\0" correct? That sounds right. > I would suggest that maybe rather than allowing the code to malloc() any size > buffer to match the incoming element length that instead or in addition we > limit the element length as well. Allowing the code to malloc() an unlimited > buffer can also lead to security issues. Since the original code defined a > 256 byte buffer, 256 seems to be a good limit for elements of the path. It > might be a better idea to simply add the limit checking rather than doing a > lot of malloc and free calls. If an element is larger than the limit, then > produce a mal-formed error message and bail out. Well, part of the original problem was that a static-length buffer was used, and there wasn't proper bounds checking in the first place. ;-) > > Also I don't think that the current patch will work without the multi-path > patch that was also submitted, without being reworked. The current patch > removes the recursive call to process_path() which means that without the for > loop that was provided in the multi-path patch, only one element of the path > will be processed and the rest will be ignored. The current patch probably > needs to be reworked and separated from the multi-path patch. For reference, most filesystem don't allow individual file or directory names to be longer than 255 characters. However, the full path can be much longer... -- Jesse Becker GPG Fingerprint -- BD00 7AA4 4483 AFCC 82D0 2720 0083 0931 9A2B 06A2 -- This SF.net email is sponsored by: SourcForge Community SourceForge wants to tell your story. http://p.sf.net/sfu/sf-spreadtheword ___ Ganglia-developers mailing list Ganglia-developers@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/ganglia-developers
Re: [Ganglia-developers] Patch for php logging noise.
Any comments? Can this be committed to SVN? I would do it myself, but I do not have permission. ~Jason On Tue, 2009-01-13 at 09:57 -0500, Jason A. Smith wrote: > This patch comes from a colleague of mine (Jerome Lauret) who said he > was getting a lot of noise in his apache logs from ganglia's php. I > didn't test this myself, but it looks pretty harmless since it just does > a few simple sanity checks on some variables before assigning values. > > ~Jason > > > -- > This SF.net email is sponsored by: > SourcForge Community > SourceForge wants to tell your story. > http://p.sf.net/sfu/sf-spreadtheword > ___ > Ganglia-developers mailing list > Ganglia-developers@lists.sourceforge.net > https://lists.sourceforge.net/lists/listinfo/ganglia-developers -- /--\ | Jason A. Smith Email: smit...@bnl.gov | | Atlas Computing Facility, Bldg. 510MPhone: +1-631-344-4226 | | Brookhaven National Lab, P.O. Box 5000 Fax: +1-631-344-7616 | | Upton, NY 11973-5000, U.S.A. | \--/ smime.p7s Description: S/MIME cryptographic signature -- This SF.net email is sponsored by: SourcForge Community SourceForge wants to tell your story. http://p.sf.net/sfu/sf-spreadtheword___ Ganglia-developers mailing list Ganglia-developers@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/ganglia-developers
Re: [Ganglia-developers] patches for: [Sec] Gmetad server BoF andnetwork overload + [Feature] multiple requests per conn oninteractive port
>>> On 1/14/2009 at 3:10 PM, in message , "Jesse Becker" wrote: > On Wed, Jan 14, 2009 at 12:56, Brad Nicholes wrote: >> Sounds good, can we condense the description below into something that we > can put into the 3.1 STATUS file so that we can start the process of getting > this backported into 3.1? > > Done. Committed as r1947. > > I suggest that once this is accepted, we release 3.1.2 ASAP. > > Also, a big thanks to Spike for finding and tracking this problem down. Spike, Correct me if I am wrong, but I think we still have a one-off problem with the patch. len = q-p; /* +1 not needed as q-p is already accounting for that */ element = malloc(len); strncpy(element, p, len); element[len] = '\0'; In the above code suppose "len" is 10 so we malloc() 10 bytes which means that the array indexing for element is 0-9. Then we strncpy() 10 bytes into "element" which is OK. However setting element[len] = "\0", IOW element[10] = "\0" is indexing into the element array 1 byte beyond the actual length of element. So either the code needs to be element = malloc(len+1) or element[len-1] = "\0" correct? I would suggest that maybe rather than allowing the code to malloc() any size buffer to match the incoming element length that instead or in addition we limit the element length as well. Allowing the code to malloc() an unlimited buffer can also lead to security issues. Since the original code defined a 256 byte buffer, 256 seems to be a good limit for elements of the path. It might be a better idea to simply add the limit checking rather than doing a lot of malloc and free calls. If an element is larger than the limit, then produce a mal-formed error message and bail out. Also I don't think that the current patch will work without the multi-path patch that was also submitted, without being reworked. The current patch removes the recursive call to process_path() which means that without the for loop that was provided in the multi-path patch, only one element of the path will be processed and the rest will be ignored. The current patch probably needs to be reworked and separated from the multi-path patch. Brad -- This SF.net email is sponsored by: SourcForge Community SourceForge wants to tell your story. http://p.sf.net/sfu/sf-spreadtheword ___ Ganglia-developers mailing list Ganglia-developers@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/ganglia-developers
