link: Check buffer length when parsing NLAs

[Jeff-A-Martin]

Verify the NlaBuffer has the expected size when parsing the IFLA_XDP_ATTACHED and IFLA_VLAN_QOS_MAPPING interface NLAs. This prevents a panic opportunity when attempting to parse malformed interface NLAs.

[Jeff-A-Martin]

This PR fixes issue #100

[Jeff-A-Martin]

@cathay4t Would you be available to review this PR?

[codecov[bot]]

Codecov Report

Attention: Patch coverage is 90.90909% with 1 lines in your changes are missing coverage. Please review.

Project coverage is 65.54%. Comparing base (4d5f819) to head (0372634).

Files	Patch %	Lines
src/link/link_info/vlan.rs	87.50%	1 Missing :warning:

Additional details and impacted files

```diff @@ Coverage Diff @@ ## main #101 +/- ## ========================================== - Coverage 65.78% 65.54% -0.25% ========================================== Files 140 140 Lines 8991 8788 -203 ========================================== - Hits 5915 5760 -155 + Misses 3076 3028 -48 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

[Jeff-A-Martin]

I see that the clippy check fails against this PR. It looks like an issue that exists a head, and @little-dude sent out a PR to fix it: https://github.com/rust-netlink/netlink-packet-route/pull/105. Thanks!

[cathay4t]

In linux kernel we have [IFLA_VLAN_QOS_MAPPING] = { .len = sizeof(struct ifla_vlan_qos_mapping) } and

struct ifla_vlan_qos_mapping {
    __u32 from;
    __u32 to;
};

And this struct has never changed since 2012-10-13 (607ca46e97a1b6594b29647d98a32d545c24bdff).

From my point of view, your PR just convert the panic to a error instead of actual fixing the problem.

Please investigate the root cause of your panic on range end index 4 out of range for slice of length 3, then we can provide proper fix. If it is hard for you to read linux kernel code, please provider reproducer of your problem, I can look into it in my free time.

[cathay4t]

And also we have [IFLA_XDP_ATTACHED] = { .type = NLA_U8 } in kernel.

[cathay4t]

If this is for non-linux OS, please protect it with #[cfg(target_os = "fuchsia")]

[Jeff-A-Martin]

You're comment "If this is for non-linux OS ..." is correct, this occurs on Fuchsia OS. However our use-case there is perhaps a bit unconventional: We use this crate inside of the OS to parse user provided Netlink messages from bytes into well formed Rust types. In that situation, panics are quite catastrophic (because we crash the Netlink implementation in the OS), and errors are very useful because we can surface them back to the user (e.g. via a NACK containing EINVAL).

I'm happy to add the #[cfg(target_os = "fuchsia")] guards if you prefer that approach; However, I do wonder if that limits the utility of this fix on other platforms. This issue could happen on other platforms in any case where you try to parse a Netlink message from bytes that came from an "unreliable source" (i.e. not the Linux kernel). What do you think?

[cathay4t]

You're comment "If this is for non-linux OS ..." is correct, this occurs on Fuchsia OS. However our use-case there is perhaps a bit unconventional: We use this crate inside of the OS to parse user provided Netlink messages from bytes into well formed Rust types. In that situation, panics are quite catastrophic (because we crash the Netlink implementation in the OS), and errors are very useful because we can surface them back to the user (e.g. via a NACK containing EINVAL).

I'm happy to add the #[cfg(target_os = "fuchsia")] guards if you prefer that approach; However, I do wonder if that limits the utility of this fix on other platforms. This issue could happen on other platforms in any case where you try to parse a Netlink message from bytes that came from an "unreliable source" (i.e. not the Linux kernel). What do you think?

I agree a library/crate should never panic and it is valid point on this crate acted as netlink server instead of parsing from realizable source.

I have also write this use case as ADR(Architecture Decision Record):

https://github.com/rust-netlink/.github/blob/main/architecture_decisions/ADR-002-non-linux-support.md

I am afraid there will be massive changes required to meet your goal on parsing data from hostile source. The one I can point is XxxxBuffer::new() need to replaced with XxxBuffer::new_checked().

Thanks for the explaination! I will merge it.