Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fixes a use-of-uninitialized-value in light_pcapng.c #1669

Open
wants to merge 4 commits into
base: dev
Choose a base branch
from
+132 −117
Open

fixes a use-of-uninitialized-value in light_pcapng.c #1669

Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
60323c3
Update light_pcapng.c
Shivam7-1 Dec 23, 2024
cd2cb51
Fixes Formatting issues
Shivam7-1 Dec 25, 2024
88659d7
Freed opt Memory Before Reassignment
Shivam7-1 Dec 25, 2024
9f355b1
Fixes memory leakages
Shivam7-1 Dec 25, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
119 changes: 55 additions & 64 deletions 3rdParty/LightPcapNg/LightPcapNg/src/light_pcapng.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -85,56 +85,52 @@ static struct _light_option *__parse_options(uint32_t **memory, const int32_t ma
/// <param name="current">Block pointer with type and length filled out</param>
/// <param name="local_data">Pointer to data which constitutes block body</param>
/// <param name="block_start">Pointer to the start of the block data</param>
void parse_by_block_type(struct _light_pcapng *current, const uint32_t *local_data, const uint32_t *block_start)
{
switch (current->block_type)
{
case LIGHT_SECTION_HEADER_BLOCK:
{
DPRINT_HERE(LIGHT_SECTION_HEADER_BLOCK);
struct _light_section_header *shb = calloc(1, sizeof(struct _light_section_header));
struct _light_option *opt = NULL;
uint32_t version;
int32_t local_offset;

shb->byteorder_magic = *local_data++;
// TODO check byte order.
version = *local_data++;
shb->major_version = version & 0xFFFF;
shb->minor_version = (version >> 16) & 0xFFFF;
shb->section_length = *((uint64_t*)local_data);
local_data += 2;

current->block_body = (uint32_t*)shb;
local_offset = (size_t)local_data - (size_t)block_start;
opt = __parse_options((uint32_t **)&local_data, current->block_total_length - local_offset - sizeof(current->block_total_length));
current->options = opt;
}
break;

case LIGHT_INTERFACE_BLOCK:
{
DPRINT_HERE(LIGHT_INTERFACE_BLOCK);
struct _light_interface_description_block *idb = calloc(1, sizeof(struct _light_interface_description_block));
struct _light_option *opt = NULL;
uint32_t link_reserved = *local_data++;
int32_t local_offset;

idb->link_type = link_reserved & 0xFFFF;
idb->reserved = (link_reserved >> 16) & 0xFFFF;
idb->snapshot_length = *local_data++;
current->block_body = (uint32_t*)idb;
local_offset = (size_t)local_data - (size_t)block_start;
opt = __parse_options((uint32_t **)&local_data, current->block_total_length - local_offset - sizeof(current->block_total_length));
current->options = opt;
}
break;
void parse_by_block_type(struct _light_pcapng *current, const uint32_t *local_data, const uint32_t *block_start) {
switch (current->block_type) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's better to follow the original format.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for reviewing Could you clarify if the concern is solely about formatting or if there are issues with the fix itself? If it’s just formatting, I’ll align it with the original style while keeping the vulnerability fix intact.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Only about the formatting. We try to minimize the change in 3rdParty because we may merge the latest changes from upstream in the future. However, vulnerability fixes are welcome. Please also leave a comment like // PCPP patch in your patch so we can easily identify the changes in the future.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes Done you can Review and Ready to Merge

case LIGHT_SECTION_HEADER_BLOCK: {
DPRINT_HERE(LIGHT_SECTION_HEADER_BLOCK);
struct _light_section_header *shb = calloc(1, sizeof(struct _light_section_header));
struct _light_option *opt = calloc(1, sizeof(struct _light_option));
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Doesn't that esentially leak the memory as opt is overwritten on L109?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes you are correct for this here can be used free (opt) Memory Before Reassignment
I had created Commit below

uint32_t version = 0;
int32_t local_offset = 0;

shb->byteorder_magic = *local_data++;
// TODO check byte order.
version = *local_data++;
shb->major_version = version & 0xFFFF;
shb->minor_version = (version >> 16) & 0xFFFF;
shb->section_length = *((uint64_t*)local_data);
local_data += 2;

current->block_body = (uint32_t*)shb;
local_offset = (size_t)local_data - (size_t)block_start;
opt = __parse_options((uint32_t **)&local_data, current->block_total_length - local_offset - sizeof(current->block_total_length));
current->options = opt;
}
break;

case LIGHT_INTERFACE_BLOCK: {
DPRINT_HERE(LIGHT_INTERFACE_BLOCK);
struct _light_interface_description_block *idb = calloc(1, sizeof(struct _light_interface_description_block));
struct _light_option *opt = calloc(1, sizeof(struct _light_option));
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ditto: memory leak

uint32_t link_reserved = *local_data++;
int32_t local_offset = 0;

idb->link_type = link_reserved & 0xFFFF;
idb->reserved = (link_reserved >> 16) & 0xFFFF;
idb->snapshot_length = *local_data++;
current->block_body = (uint32_t*)idb;
local_offset = (size_t)local_data - (size_t)block_start;
opt = __parse_options((uint32_t **)&local_data, current->block_total_length - local_offset - sizeof(current->block_total_length));
current->options = opt;
}
break;

case LIGHT_ENHANCED_PACKET_BLOCK:
{
DPRINT_HERE(LIGHT_ENHANCED_PACKET_BLOCK);
struct _light_enhanced_packet_block *epb = NULL;
struct _light_option *opt = NULL;
struct _light_option *opt = calloc(1, sizeof(struct _light_option));
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ditto: memory leak

uint32_t interface_id = *local_data++;
uint32_t timestamp_high = *local_data++;
uint32_t timestamp_low = *local_data++;
Expand Down Expand Up @@ -182,7 +178,7 @@ void parse_by_block_type(struct _light_pcapng *current, const uint32_t *local_da
{
DPRINT_HERE(LIGHT_CUSTOM_DATA_BLOCK);
struct _light_custom_nonstandard_block *cnb = NULL;
struct _light_option *opt = NULL;
struct _light_option *opt = calloc(1, sizeof(struct _light_option));
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ditto: memory leak

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes done some Improvement to memory free(opt) now there wouldn't be any leakage i think

uint32_t len = *local_data++;
uint32_t reserved0 = *local_data++;
uint32_t reserved1 = *local_data++;
Expand All @@ -203,24 +199,19 @@ void parse_by_block_type(struct _light_pcapng *current, const uint32_t *local_da
current->options = opt;
}
break;

default: // Could not find registered block type. Copying data as RAW.
{
DPRINT_HERE(default);
uint32_t raw_size = current->block_total_length - 2 * sizeof(current->block_total_length) - sizeof(current->block_type);
if (raw_size > 0)
{
current->block_body = calloc(raw_size, 1);
memcpy(current->block_body, local_data, raw_size);
local_data += raw_size / (sizeof(*local_data));
}
else
{
current->block_body = NULL;
}
}
break;
}
default: {
DPRINT_HERE(default);
uint32_t raw_size = current->block_total_length - 2 * sizeof(current->block_total_length) - sizeof(current->block_type);
if (raw_size > 0) {
current->block_body = calloc(raw_size, 1);
memcpy(current->block_body, local_data, raw_size);
local_data += raw_size / (sizeof(*local_data));
} else {
current->block_body = NULL;
}
}
break;
}
}

// Parse memory and allocate _light_pcapng array.
Expand Down
Loading