[quagga-dev 3076] Re: bugs related to length of interface names
Andrew J. Schorr
aschorr at telemetry-investments.com
Fri Apr 1 18:58:12 BST 2005
On Fri, Apr 01, 2005 at 06:38:19PM +0100, Paul Jakma wrote:
> On Fri, 1 Apr 2005, Andrew J. Schorr wrote:
> Ok, but that would not be a matter of not accepting overlong string
> in the first place? If you do that then ifname as "nul terminated
> string up to INTERFACE_NAMSIZ bytes in size, not including the nul"
> is consistent no?
The policy regarding the ifname inside struct interface is already clear
and consistent: it can contain up to INTERFACE_NAMSIZ bytes, and it
is always NUL-terminated.
That is not the problem. The problem is that there is not a clear
and consistent policy regarding the strings that are passed to
if_lookup_by_name, if_get_by_name, and if_create.
For example, look in zebra/if_ioctl.c:interface_list_ioctl().
It includes a line that looks like this:
ifp = if_get_by_name (ifreq->ifr_name);
Is that valid? Or is it a bug? I don't know. Is ifreq->ifr_name
guaranteed to have a NUL termination char? If it is, then the code
is probably OK, except for the risk of false matches if the length
somehow exceeds INTERFACE_NAMSIZ. But if it is a fixed-size buffer
of length IFNAMSIZ, then this call could run off the end of the buffer.
If the API required the caller to specify the length, we wouldn't have
to wonder whether this code is correct...
> The nul will always be there, at or before offset INTERFACE_NAMSIZ.
Yes, that is true of the ifname inside the structure, but not those
that are roaming in the wild.
> >But if it is coming from the command line or a config file, the proper
> >call is (since there is no limit on the string length):
> > if_lookup_by_name_len(name, strlen(name))
> So do the check when validating the user supplied name. After that we
> dont care, see the constraint above. No?
Yes, we could do that. But there are a lot of places. Do you really
want to add a check in every single "show interface IFNAME" or
"show ip ospf interface IFNAME" or ospf6 versions, etc.?
If you grep the source tree for "interface IFNAME" (which is the canonical
string used in declaring a command that takes an interface name as a
parameter), there are 32 instances in the source code. I don't personally
want to add a check in each case. It is much easier simply to
replace if_lookup_by_name(argv) with
> But if INTERFACE_NAMSIZ is always >= IFNAMSIZ, and interface name is
> always stored in a INTERFACE_NAMSIZ+1 array, always nul-terminated,
> why bother exactly?
Because suppose the kernel hands us an interface name of exactly IFNAMSIZ
bytes that does not have a terminating NUL character. If we then
call if_lookup_by_name on that string, then this line of code:
if (strncmp (name, ifp->name, sizeof ifp->name) == 0)
can run off the end of the kernel-supplied string in the case where
ifp->name actually has more than IFNAMSIZ bytes (since INTERFACE_NAMSIZ is
larger than IFNAMSIZ). Of course, you may ask how that could happen in
the first place, but it could if the user creates a pseudo-interface
that has a name longer than IFNAMSIZ bytes...
> Or am i being dense? :)
No, I don't think so, I find this all a bit confusing. My thought is
just to make the API require explicitly counted strings so we don't
have to worry about subtle problems.
At the moment, I don't think there are any problems if all the interface
names are less than IFNAMSIZ in length. But if they go over that limit,
there may be subtle bugs and/or unexpected behavior lurking in the code.
More information about the Quagga-dev