Re: [flac-dev] [PATCH] Fix buffer overflow in metaflac

2012-04-06 Thread Earl Chew 
It would really be better to compare against sizeof(application_id) rather than 
coupling to all these
instances of 4 all over the place.




 From: Erik de Castro Lopo mle...@mega-nerd.com
To: flac-dev@xiph.org 
Sent: Thursday, April 5, 2012 4:02:10 AM
Subject: Re: [flac-dev] [PATCH] Fix buffer overflow in metaflac
 
Cristian Rodríguez wrote:

 strlen() returns the length excluding the terminating null byte..then
 an string of len 4 will be off-by-one in application_id[4];
 
 GCC 4.7 detects this bug.

Ah nice!

 diff --git a/src/metaflac/options.c b/src/metaflac/options.c
 index eb3498d..2cb0959 100644
 --- a/src/metaflac/options.c
 +++ b/src/metaflac/options.c
 @@ -1040,7 +1040,7 @@ FLAC__bool parse_block_type(const char *in, 
 Argument_BlockType *out)
              out-entries[entry].type = FLAC__METADATA_TYPE_APPLICATION;
              out-entries[entry].filter_application_by_id = (0 != r);
              if(0 != r) {
 -                if(strlen(r) == 4) {
 +                if(strlen(r) == 3) {
                      strcpy(out-entries[entry].application_id, r);
                  }


I actually think that this is a better solution:

                                if(strlen(r) == 4) {
-                                       
strcpy(out-entries[entry].application_id, r);
+                                       
memcpy(out-entries[entry].application_id, r, 4);
                                }


Cheers,
Erik
-- 
--
Erik de Castro Lopo
http://www.mega-nerd.com/
___
flac-dev mailing list
flac-dev@xiph.org
http://lists.xiph.org/mailman/listinfo/flac-dev___
flac-dev mailing list
flac-dev@xiph.org
http://lists.xiph.org/mailman/listinfo/flac-dev

Re: [flac-dev] [PATCH] Fix buffer overflow in metaflac

2012-04-05 Thread Erik de Castro Lopo 
Cristian Rodríguez wrote:

 strlen() returns the length excluding the terminating null byte..then
 an string of len 4 will be off-by-one in application_id[4];
 
 GCC 4.7 detects this bug.

Ah nice!

 diff --git a/src/metaflac/options.c b/src/metaflac/options.c
 index eb3498d..2cb0959 100644
 --- a/src/metaflac/options.c
 +++ b/src/metaflac/options.c
 @@ -1040,7 +1040,7 @@ FLAC__bool parse_block_type(const char *in, 
 Argument_BlockType *out)
   out-entries[entry].type = 
 FLAC__METADATA_TYPE_APPLICATION;
   out-entries[entry].filter_application_by_id = (0 != r);
   if(0 != r) {
 - if(strlen(r) == 4) {
 + if(strlen(r) == 3) {
   
 strcpy(out-entries[entry].application_id, r);
   }


I actually think that this is a better solution:

if(strlen(r) == 4) {
-   
strcpy(out-entries[entry].application_id, r);
+   
memcpy(out-entries[entry].application_id, r, 4);
}


Cheers,
Erik
-- 
--
Erik de Castro Lopo
http://www.mega-nerd.com/
___
flac-dev mailing list
flac-dev@xiph.org
http://lists.xiph.org/mailman/listinfo/flac-dev

Re: [flac-dev] [PATCH] Fix buffer overflow in metaflac

2012-04-05 Thread Brian Willoughby 
Agreed.  I was going to suggest memcpy() or something equivalent,  
because the FLAC structure is not literally a C string, but rather a  
32-bit field that may or may not have a terminating NULL.  Erik's  
code should work correctly in all cases.


On Apr 5, 2012, at 04:02, Erik de Castro Lopo wrote:
 I actually think that this is a better solution:

 if(strlen(r) == 4) {
 -   strcpy(out-entries 
 [entry].application_id, r);
 +   memcpy(out-entries 
 [entry].application_id, r, 4);
 }



___
flac-dev mailing list
flac-dev@xiph.org
http://lists.xiph.org/mailman/listinfo/flac-dev

Re: [flac-dev] [PATCH] Fix buffer overflow in metaflac

2012-04-05 Thread Cristian Rodríguez 
El 05/04/12 18:30, Brian Willoughby escribió:
 Agreed.  I was going to suggest memcpy() or something equivalent,
 because the FLAC structure is not literally a C string, but rather a
 32-bit field that may or may not have a terminating NULL.  Erik's
 code should work correctly in all cases.

Yep, and a fix is already in git ;)



___
flac-dev mailing list
flac-dev@xiph.org
http://lists.xiph.org/mailman/listinfo/flac-dev