flac dev - Apr 2012 - [PATCH] Fix buffer overflow in metaflac

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.
---
 src/metaflac/options.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

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);
 				}
 				else if(strlen(r) == 10 && strncmp(r, "0x", 2) == 0
&& strspn(r+2, "0123456789ABCDEFabcdef") == 8) {
-- 
1.7.9.2

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/

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);
>                                 }

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+la at mega-nerd.com>
To: flac-dev at 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 at xiph.org
http://lists.xiph.org/mailman/listinfo/flac-dev
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
http://lists.xiph.org/pipermail/flac-dev/attachments/20120406/7f1db663/attachment.htm