Skip to content
Permalink
Browse files Browse the repository at this point in the history
Merge pull request from GHSA-rwgw-vwxg-q799
* Prevent potential infinite loop when parsing WAV format file

* Check if subchunk is negative.

* Fix and add checks

* Change data type from pj_ssize_t to long.

* Modify check

* Fix leak file descriptor and modify check on wav_playlist

* Move overflow/underflow check to pj_file_setpos()

* Use macro to simplify check

* modification based on comments

* Remove unnecessary casting

* Modification based on comments
  • Loading branch information
trengginas committed Apr 25, 2022
1 parent d2006a4 commit 947bc1e
Show file tree
Hide file tree
Showing 5 changed files with 51 additions and 10 deletions.
8 changes: 8 additions & 0 deletions pjlib/include/pj/types.h
Expand Up @@ -31,6 +31,7 @@
* @{
*/
#include <pj/config.h>
#include <pj/limits.h>

PJ_BEGIN_DECL

Expand Down Expand Up @@ -361,6 +362,13 @@ PJ_INLINE(pj_int32_t) pj_swap32(pj_int32_t val32)
return val32;
}

/* This is to check if uint32 var will overflow if converted to signed long */
#define PJ_CHECK_OVERFLOW_UINT32_TO_LONG(uint32_var, exec_on_overflow) \
do { \
if (uint32_var > PJ_MAXLONG) { \
exec_on_overflow; \
} \
} while (0)

/**
* @}
Expand Down
7 changes: 7 additions & 0 deletions pjlib/src/pj/file_io_ansi.c
Expand Up @@ -20,6 +20,7 @@
#include <pj/file_io.h>
#include <pj/assert.h>
#include <pj/errno.h>
#include <pj/limits.h>
#include <stdio.h>
#include <errno.h>

Expand Down Expand Up @@ -124,6 +125,12 @@ PJ_DEF(pj_status_t) pj_file_setpos( pj_oshandle_t fd,
{
int mode;

if ((sizeof(pj_off_t) > sizeof(long)) &&
(offset > PJ_MAXLONG || offset < PJ_MINLONG))
{
return PJ_ENOTSUP;
}

switch (whence) {
case PJ_SEEK_SET:
mode = SEEK_SET; break;
Expand Down
22 changes: 18 additions & 4 deletions pjmedia/src/pjmedia/avi_player.c
Expand Up @@ -282,7 +282,7 @@ pjmedia_avi_player_create_streams(pj_pool_t *pool,
/* Read the headers of each stream. */
for (i = 0; i < avi_hdr.avih_hdr.num_streams; i++) {
pj_size_t elem = 0;
pj_ssize_t size_to_read;
pj_off_t size_to_read;

/* Read strl header */
status = file_read(fport[0]->fd, &avi_hdr.strl_hdr[i],
Expand Down Expand Up @@ -335,6 +335,7 @@ pjmedia_avi_player_create_streams(pj_pool_t *pool,
do {
pjmedia_avi_subchunk ch;
int read = 0;
pj_off_t size_to_read;

status = file_read(fport[0]->fd, &ch, sizeof(pjmedia_avi_subchunk));
if (status != PJ_SUCCESS) {
Expand All @@ -349,7 +350,15 @@ pjmedia_avi_player_create_streams(pj_pool_t *pool,
break;
}

status = pj_file_setpos(fport[0]->fd, ch.len-read, PJ_SEEK_CUR);
if (ch.len < read) {
status = PJ_EINVAL;
goto on_error;
}
PJ_CHECK_OVERFLOW_UINT32_TO_LONG(ch.len - read,
status = PJ_EINVAL; goto on_error;);
size_to_read = (pj_off_t)ch.len - read;

status = pj_file_setpos(fport[0]->fd, size_to_read, PJ_SEEK_CUR);
if (status != PJ_SUCCESS) {
goto on_error;
}
Expand Down Expand Up @@ -775,6 +784,8 @@ static pj_status_t avi_get_frame(pjmedia_port *this_port,
/* Read new chunk data */
if (fport->size_left == 0) {
pj_off_t pos;
pj_off_t ch_len;

pj_file_getpos(fport->fd, &pos);

/* Data is padded to the nearest WORD boundary */
Expand All @@ -788,6 +799,10 @@ static pj_status_t avi_get_frame(pjmedia_port *this_port,
size_read = 0;
goto on_error2;
}

PJ_CHECK_OVERFLOW_UINT32_TO_LONG(ch.len,
status = PJ_EINVAL; goto on_error2;);
ch_len = ch.len;

cid = (char *)&ch.id;
if (cid[0] >= '0' && cid[0] <= '9' &&
Expand All @@ -814,8 +829,7 @@ static pj_status_t avi_get_frame(pjmedia_port *this_port,
goto on_error2;
}

status = pj_file_setpos(fport->fd, ch.len,
PJ_SEEK_CUR);
status = pj_file_setpos(fport->fd, ch_len, PJ_SEEK_CUR);
continue;
}
fport->size_left = ch.len;
Expand Down
12 changes: 9 additions & 3 deletions pjmedia/src/pjmedia/wav_player.c
Expand Up @@ -188,7 +188,8 @@ PJ_DEF(pj_status_t) pjmedia_wav_player_port_create( pj_pool_t *pool,
pjmedia_port **p_port )
{
pjmedia_wave_hdr wave_hdr;
pj_ssize_t size_to_read, size_read;
pj_ssize_t size_read;
pj_off_t size_to_read;
struct file_reader_port *fport;
pjmedia_audio_format_detail *ad;
pj_off_t pos;
Expand Down Expand Up @@ -234,7 +235,7 @@ PJ_DEF(pj_status_t) pjmedia_wav_player_port_create( pj_pool_t *pool,
return status;

/* Read the file header plus fmt header only. */
size_read = size_to_read = sizeof(wave_hdr) - 8;
size_to_read = size_read = sizeof(wave_hdr) - 8;
status = pj_file_read( fport->fd, &wave_hdr, &size_read);
if (status != PJ_SUCCESS) {
pj_file_close(fport->fd);
Expand Down Expand Up @@ -297,7 +298,9 @@ PJ_DEF(pj_status_t) pjmedia_wav_player_port_create( pj_pool_t *pool,
* fmt header data.
*/
if (wave_hdr.fmt_hdr.len > 16) {
size_to_read = wave_hdr.fmt_hdr.len - 16;
PJ_CHECK_OVERFLOW_UINT32_TO_LONG(wave_hdr.fmt_hdr.len - 16,
pj_file_close(fport->fd); return PJMEDIA_ENOTVALIDWAVE;);
size_to_read = (pj_off_t)wave_hdr.fmt_hdr.len - 16;
status = pj_file_setpos(fport->fd, size_to_read, PJ_SEEK_CUR);
if (status != PJ_SUCCESS) {
pj_file_close(fport->fd);
Expand Down Expand Up @@ -326,7 +329,10 @@ PJ_DEF(pj_status_t) pjmedia_wav_player_port_create( pj_pool_t *pool,
}

/* Otherwise skip the chunk contents */
PJ_CHECK_OVERFLOW_UINT32_TO_LONG(subchunk.len,
pj_file_close(fport->fd); return PJMEDIA_ENOTVALIDWAVE;);
size_to_read = subchunk.len;

status = pj_file_setpos(fport->fd, size_to_read, PJ_SEEK_CUR);
if (status != PJ_SUCCESS) {
pj_file_close(fport->fd);
Expand Down
12 changes: 9 additions & 3 deletions pjmedia/src/pjmedia/wav_playlist.c
Expand Up @@ -419,7 +419,8 @@ PJ_DEF(pj_status_t) pjmedia_wav_playlist_create(pj_pool_t *pool,
for (index=file_count-1; index>=0; index--) {

pjmedia_wave_hdr wavehdr;
pj_ssize_t size_to_read, size_read;
pj_ssize_t size_read;
pj_off_t size_to_read;

/* we end with the last one so we are good to go if still in function*/
pj_memcpy(filename, file_list[index].ptr, file_list[index].slen);
Expand All @@ -442,7 +443,7 @@ PJ_DEF(pj_status_t) pjmedia_wav_playlist_create(pj_pool_t *pool,
goto on_error;

/* Read the file header plus fmt header only. */
size_read = size_to_read = sizeof(wavehdr) - 8;
size_to_read = size_read = sizeof(wavehdr) - 8;
status = pj_file_read( fport->fd_list[index], &wavehdr, &size_read);
if (status != PJ_SUCCESS) {
goto on_error;
Expand Down Expand Up @@ -492,7 +493,9 @@ PJ_DEF(pj_status_t) pjmedia_wav_playlist_create(pj_pool_t *pool,
* fmt header data.
*/
if (wavehdr.fmt_hdr.len > 16) {
size_to_read = wavehdr.fmt_hdr.len - 16;
PJ_CHECK_OVERFLOW_UINT32_TO_LONG(wavehdr.fmt_hdr.len-16,
status = PJMEDIA_ENOTVALIDWAVE; goto on_error;);
size_to_read = (pj_off_t)wavehdr.fmt_hdr.len - 16;
status = pj_file_setpos(fport->fd_list[index], size_to_read,
PJ_SEEK_CUR);
if (status != PJ_SUCCESS) {
Expand Down Expand Up @@ -522,7 +525,10 @@ PJ_DEF(pj_status_t) pjmedia_wav_playlist_create(pj_pool_t *pool,
}

/* Otherwise skip the chunk contents */
PJ_CHECK_OVERFLOW_UINT32_TO_LONG(subchunk.len,
status = PJMEDIA_ENOTVALIDWAVE; goto on_error;);
size_to_read = subchunk.len;

status = pj_file_setpos(fport->fd_list[index], size_to_read,
PJ_SEEK_CUR);
if (status != PJ_SUCCESS) {
Expand Down

0 comments on commit 947bc1e

Please sign in to comment.