afinet: add option to set the maximum spoofed datagram size

[bazsi]

Signed-off-by: Balazs Scheidler [email protected]

[kira-syslogng]

Build SUCCESS

[xtaran]

While I haven't checked if the same issue is present when using IPv6 nor if that afinet driver is IPv4 only, it looks to me as if this code is hardcoding IPv4-specific values:

/* maximum ip datagram is 65535, minus IP + UDP header */
self->spoof_source_max_msglen = MAX(0, MIN(max_msglen, 65535 - 20 - 8));

So if not all we're looking at here is IPv4-only, this might cause some issues with UDP over IPv6.

[bazsi]

You are right these are ipv4 specific values, but in reality these don't really matter that much. In reality, anything that is more than a very few fragments will be practically useless in production scenarios. I can't remember the exact reasoning for the truncation, but my vague recollection is that anything that starts to use IP fragments start to cause message drops drastically. And in case a fragment is dropped, the entire UDP payload will be dropped. And 1024 was the limit in RFC3164.

…

On Tue, Feb 5, 2019 at 5:45 PM Axel Beckert ***@***.***> wrote: While I haven't checked if the same issue is present when using IPv6 nor if that afinet driver is IPv4 only, it looks to me as if this code is hardcoding IPv4-specific values: /* maximum ip datagram is 65535, minus IP + UDP header */ self->spoof_source_max_msglen = MAX(0, MIN(max_msglen, 65535 - 20 - 8)); So if not all we're looking at here is IPv4-only, this might cause some issues with UDP over IPv6. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#2535 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AArldoy3b4JTfMGCCxeA1XkCF3x9omijks5vKbUZgaJpZM4ajkUI> .

[xtaran]

You are right these are ipv4 specific values, but in reality these don't really matter that much. In reality, anything that is more than a very few fragments will be practically useless in production scenarios.

True indeed.

My thought was though more going towards a potential buffer overflow or memory leak if this maximal allowed size is used in conjunction with IPv6 UDP packets… The current value seems to be the theoretically biggest usable size. (Which is probably be totally ok if using this with IPv6 packets doesn't cause any memory management issues — but I'm not versed enough with C to determine if this is the case.)

I can't remember the exact reasoning for the truncation, […] And 1024 was the limit in RFC3164.

I suspected the latter as cause: The old syslog RFC 3164 indeed limits the size to 1024 bytes. And it also has a requirement to make relayed syslog messages adhering to the RFC if they didn't do before. These two rules together demands such a — from nowadays point of view unexpected — behaviour.

Anyway, thanks for caring and looking into this!

[gaborznagy]

Do we need to make this value configurable by users?
How about extending the max. value if we need a limit at all.
By adding an option to control this setting:

• the user should always take care of the set limit, and still can hit that limit
• it increases configuration complexity

As I see it after reading the linked RFC3164 parts, that we should only limit the length if we have to force RFC3164 format and not always.

[xtaran]

As I see it after reading the linked RFC3164 parts, that we should only limit the length if we have to force RFC3164 format and not always.

Sounds ok for me, too.

[gaborznagy]

Open point: #2535 (comment)

[lbudai]

I agree with @gaborznagy regarding to the warning message, or if we think that this max size is also known to the users, then we could document that we check the input.

[bazsi]

I have now added a warning, although I don't see it really adds any value. I don't expect anyone hitting that. But with that this should be ok for merging.

[kira-syslogng]

Build SUCCESS

[gaborznagy]

Thanks for adding the warning on the setter function!

I have given some thoughts to my previous comment:

Do we need to make this value configurable by users?
How about extending the max. value if we need a limit at all.
By adding an option to control this setting:

the user should always take care of the set limit, and still can hit that limit
it increases configuration complexity
As I see it after reading the linked RFC3164 parts, that we should only limit the length if we have to force RFC3164 format and not always.

I see that we need a limit, and we cannot guess the max value as it is environment-related, so this should be set by the user.

I was thinking to have a warning in case we truncate the UDP packet.
However if we consider that this max value is forced by the environment which cannot be changed, then the warning messages wouldn't help at all.
It would be just noise in the internal logs.
And I think the truncate feature implemented, because it was not possible to avoid fragmentation.