Menu

#118 Null Pointer Dereference in libmirage 3.2.2

None
closed
nobody
None
unassigned
high
2019-08-28
2019-08-24
No

< copy of the mail that I've sent to mrok >

Hi,
I found a Null Pointer Dereference in libmirage 3.2.2, specifically in the NRG parser.
I'm writing you directly here and not in the public bug tracker cause I'm not sure if this is a security issue or not.

The content of the file that triggers the bug is the following (12 bytes in hex):
02 80 F4 4B 4E 45 52 4F 00 00 00 04

The issue appears at the memcpy at line 103 (mirage_parser_nrg_build_block_index routine) of images/image-nrg/parser.c:

/* Populate block index */
cur_ptr = self->priv->nrg_data;
index = 0;
do {
    blockentry = g_new0(NRGBlockIndexEntry, 1);
    if (!blockentry) {
        MIRAGE_DEBUG(self, MIRAGE_DEBUG_WARNING, "%s: failed to allocate memory for block index!\n", __debug__);
        g_set_error(error, MIRAGE_ERROR, MIRAGE_ERROR_PARSER_ERROR, Q_("Failed to allocate memory for block index!"));
        return FALSE;
    }
    blockentry->offset = (guint64) (cur_ptr - self->priv->nrg_data);
    memcpy(blockentry->block_id, cur_ptr, 4); //// ISSUE HERE
    cur_ptr += 4;

==15389== Invalid read of size 4
==15389== at 0x87E2A72: mirage_parser_nrg_build_block_index (parser.c:103)
==15389== by 0x87E2A72: mirage_parser_nrg_load_image (parser.c:1003)
==15389== by 0x4E5E285: mirage_context_load_image (context.c:392)
==15389== by 0x400ED9: main (driver.c:13)
==15389== Address 0x0 is not stack'd, malloc'd or (recently) free'd

The variable cur_ptr is NULL. The root cause is at line 992 (mirage_parser_nrg_load_image routine):

/* Read descriptor data */
self->priv->nrg_data = g_malloc(self->priv->nrg_data_length);
mirage_stream_seek(self->priv->nrg_stream, trailer_offset, G_SEEK_SET, NULL);
if (mirage_stream_read(self->priv->nrg_stream, self->priv->nrg_data, self->priv->nrg_data_length, NULL) != self->priv->nrg_data_length) {
    MIRAGE_DEBUG(self, MIRAGE_DEBUG_WARNING, "%s: failed to read descriptor!\n", __debug__);
    g_set_error(error, MIRAGE_ERROR, MIRAGE_ERROR_IMAGE_FILE_ERROR, Q_("Failed to read descriptor!"));
    succeeded = FALSE;
    goto end;
}

g_malloc, as opposite of the standard malloc, returns NULL when its argument is 0.
So when self->priv->nrg_data_length is 0 g_malloc returns NULL and generates the bug.

My proposed fix is the following:

--- parser_old.c 2019-08-23 20:28:29.036521000 +0200
+++ parser.c 2019-08-23 20:34:24.236933435 +0200
@@ -988,6 +988,14 @@
is a MTYP block provided */
mirage_disc_set_medium_type(self->priv->disc, MIRAGE_MEDIUM_CD);

  • / Length integrity check /
  • if (self->priv->nrg_data_length == 0) {
  • MIRAGE_DEBUG(self, MIRAGE_DEBUG_WARNING, "%s: rg_data_length must be greater than 0!\n", debug);
  • g_set_error(error, MIRAGE_ERROR, MIRAGE_ERROR_IMAGE_FILE_ERROR, Q_("nrg_data_length must be greater than 0!"));
  • succeeded = FALSE;
  • goto end;
  • }
  • / Read descriptor data /
    self->priv->nrg_data = g_malloc(self->priv->nrg_data_length);
    mirage_stream_seek(self->priv->nrg_stream, trailer_offset, G_SEEK_SET, NULL);

I hope I was helpful, GG for the code is very clear to read.
Regards,
Andrea Fioraldi

Discussion

  • Andrea Fioraldi

    Andrea Fioraldi - 2019-08-24

    Well formatted patch (SourceForge interpreted the patch as hypertext in the issue text):

    --- parser_old.c 2019-08-23 20:28:29.036521000 +0200
    +++ parser.c 2019-08-23 20:34:24.236933435 +0200
    @@ -988,6 +988,14 @@
            is a MTYP block provided */
         mirage_disc_set_medium_type(self->priv->disc, MIRAGE_MEDIUM_CD);
    
    +    /* Length integrity check */
    +    if (self->priv->nrg_data_length == 0) {
    +        MIRAGE_DEBUG(self, MIRAGE_DEBUG_WARNING, "%s: rg_data_length must be greater than 0!\n", __debug__);
    +        g_set_error(error, MIRAGE_ERROR, MIRAGE_ERROR_IMAGE_FILE_ERROR, Q_("nrg_data_length must be greater than 0!"));
    +        succeeded = FALSE;
    +        goto end;
    +    }
    +    
         /* Read descriptor data */
         self->priv->nrg_data = g_malloc(self->priv->nrg_data_length);
         mirage_stream_seek(self->priv->nrg_stream, trailer_offset, G_SEEK_SET, NULL);
    
     

    Last edit: Andrea Fioraldi 2019-08-24
  • Rok Mandeljc

    Rok Mandeljc - 2019-08-24
    • status: open --> closed
    • Milestone: -->
     
  • Rok Mandeljc

    Rok Mandeljc - 2019-08-24

    Thanks for the detailed analysis of the issue and the patch! I have applied and pushed it to our git repository.

    Best regards,
    Rok

     
  • Rok Mandeljc

    Rok Mandeljc - 2019-08-28
    • private: Yes --> No
     

Log in to post a comment.