bsnmp: zero-fill the destination on a short ASN.1 IpAddress

[ojanerif_amd.com]

Summary

asn_get_ipaddress_raw() meant to pad the 4-byte buffer after a short copy,
but the loop bound was the already-reached len and never ran, leaving
addr[len..3] uninitialised. Pad to 4.

MFC after: 1 week

[maxim]

Yep, it looks like a brain-o. Should be harmless as both bsnmpd (main.c:511) and libbsnmp (snmpclient.c:1466) zero out a pdu.

As this is a contrib, we probably want to notify the upstream.

[maxim]

By the way, how did you find this issue?

[ojanerif_amd.com]

Just reading through the bsnmp ASN.1 decoder looking for length bugs. In the
short-IpAddress case the zero-padding loop never runs, so the trailing bytes
are left uninitialised. Agreed it's harmless in practice since the callers
zero the pdu -- just a correctness fix.

[maxim]

In D60923#1397091, @ojanerif_amd.com wrote:

Just reading through the bsnmp ASN.1 decoder looking for length bugs. In the
short-IpAddress case the zero-padding loop never runs, so the trailing bytes
are left uninitialised. Agreed it's harmless in practice since the callers
zero the pdu -- just a correctness fix.

OK, thank you. Of course, this should be fixed.

I was trying to think about automated tools to detect or, better, prevent such bugs from slipping in. It looks like a good old static analyzer paired with a LLM does a decent work.

[markj]

In D60923#1397081, @maxim wrote:

Yep, it looks like a brain-o. Should be harmless as both bsnmpd (main.c:511) and libbsnmp (snmpclient.c:1466) zero out a pdu.

As this is a contrib, we probably want to notify the upstream.

It looks like most of the recent changes in bsnmp have been directly in the src repo, but I believe @harti is the authority there.

[maxim]

In D60923#1397443, @markj wrote:

In D60923#1397081, @maxim wrote:

Yep, it looks like a brain-o. Should be harmless as both bsnmpd (main.c:511) and libbsnmp (snmpclient.c:1466) zero out a pdu.

As this is a contrib, we probably want to notify the upstream.

It looks like most of the recent changes in bsnmp have been directly in the src repo, but I believe @harti is the authority there.

From my understanding, bsnmp suit consists of contrib and non-contrib parts. My recent commits were into non-contrib. This one touches contrib part.

But I agree -- @harti input would be appreciated.

[harti]

Looks ok to me.

For me it is ok to commit directly - I'm usually synchronizing the modifications to the upstream repo.

[maxim]

Hey Osvaldo,

As we have the patch reviewed, I am happy to commit it for you. I'd suggest to adjust the commit message though just to match FreeBSD style:

bsnmp: zero-fill the destination on a short ASN.1 IpAddress

asn_get_ipaddress_raw() meant to pad the 4-byte buffer after a short copy, but the loop bound was the already-reached len and never ran, leaving addr[len..3] uninitialised. Pad to 4.

This is harmless for the in-tree consumers (bsnmpd(1) and libbsnmpclient) which zero the PDU before decoding it.

Reviewed by: harti, maxim
Approved by: harti
MFC after: 2 weeks

I made MFC longer, just my personal preference. Hope this is ok.

Let me know if the above works for you.

Maxim

[maxim]

Also, please confirm that the Author line looks good to you

Author: Osvaldo Janeri Filho <[e-mail]>

[ojanerif_amd.com]

Thanks, Maxim -- much appreciated, please go ahead and commit it.

The reworded commit message looks good to me, and the Author line is correct:

Author: Osvaldo Janeri Filho <[e-mail]>

MFC after: 2 weeks is fine. Thanks also to @harti for the review and for handling the upstream sync.