ipfw: prefixlen segfault bugfix in nptv6

[p.mousavizadeh_protonmail.com]

Summary

I found a bug in the ipfw implementation of NPTv6, and it seems that it originated from cleaning up compiler warnings in 2020 (D25456). The bug occurs when we set a prefix with a prefix length inside the ipfw nptv6 create, which leads to a segmentation fault. The segmentation fault is caused by a goto statement that dereferences a null pointer.

I wanted to fix that, but I have to break backward compatibility due to poor design. It gives the user the option to set the prefix length both inside the prefix and via the prefix length option, which is confusing (D6420#142790). Additionally, using them in the incorrect order would result in undefined behavior.

So, should I remove the prefix length options or replace the internal and external prefix length addresses with their network identifier address, which is the first address of the resulting prefix?
This implementation and my preference is for the second approach because, in NPTv6, you have to set the same prefix length for both internal and external IPv6 prefixes due to its stateless nature (RFC 6296 Sec. 3.1).
Also, since we will broke the compatibility in anyway to fix this, should I change the int_prefix and ext_prefix to int_netaddr and ext_netaddr to be less confusing?

Test Plan

The bug without this fix:

# ipfw nptv6 test create int_prefix 2a01:e140:cafe::/64 ext_prefix 2a01:e140::/64
Segmentation fault (core dumped)
# ipfw nptv6 test create int_prefix 2a01:e140:cafe::/64 ext_if vmx0
Segmentation fault (core dumped)
# ipfw nptv6 test create int_prefix 2a01:e140:cafe::/64 ext_if vmx0 prefixlen 64
Segmentation fault (core dumped)

Here is how I tested the new code:

# ipfw nptv6 test create int_prefix 2a01:e140:cafe:: ext_prefix 2a01:e140:: prefixlen 64
# ipfw nptv6 test show                                                                  
nptv6 test int_prefix 2a01:e140:cafe:: ext_prefix 2a01:e140:: prefixlen 64
# ipfw nptv6 test create int_prefix 2a01:e140:cafe:: ext_prefix 2a01:e140::/64 prefixlen 64
ipfw: Bad IPv6 network address: 2a01:e140::/64
Use prefixlen option instead
# ipfw nptv6 test create int_prefix 2a01:e140:cafe::/64 ext_prefix 2a01:e140:: prefixlen 64 
ipfw: Bad IPv6 network address: 2a01:e140:cafe::/64
Use prefixlen option instead
# ipfw nptv6 test create int_prefix 2a01:e140:cafe:: ext_prefix 2a01:e140::                
ipfw: prefixlen required
# ipfw nptv6 test create prefixlen 64 int_prefix 2a01:e140:cafe:: ext_prefix 2a01:e140::
# ipfw nptv6 test show                                                                     
nptv6 test int_prefix 2a01:e140:cafe:: ext_prefix 2a01:e140:: prefixlen 64

[adrian]

I think this looks fine. I'm personally not too worried about breaking things in ipfw to make them much cleaner/clearer/less error prone, as long as we document it in UPDATING.

[ivy]

usually i prefer CIDR prefixes over prefixlen, but since the same mask applies to both networks, i think prefixlen is better here.

the fact that no one noticed this for 5 years suggests the other syntax isn't commonly used, but it probably needs a Relnotes entry since we're breaking a (theoretically) previously supported syntax.

[p.mousavizadeh_protonmail.com]

Thank you all for the reviews.
At this point, I think this revision is ready to land. @ivy, Could you please commit it? is there anything I can help with or do to make it happen?

[p.mousavizadeh_protonmail.com]

@des @kevans
Could you please review this revision? is there anything I can help with?

[des]

I agree that the syntax is poorly designed, but it's easily fixable without breaking backward compatibility.

sbin/ipfw/ipfw.8

3837 ↗

(On Diff #156252)

There is nothing wrong with ipv6_prefix, and it's also used in several other places in the manual page, so changing it makes the text inconsistent.

3838 ↗

(On Diff #156252)

The original text was clunky, but your replacement is clunkier.

[p.mousavizadeh_protonmail.com]

In D50597#1175386, @des wrote:

I agree that the syntax is poorly designed, but it's easily fixable without breaking backward compatibility.

The reason for my proposed syntax is that in nptv6, both internal and external prefixes should have the same prefix length due to its stateless nature. That's why I don't think obtaining the prefix length from each internal and external prefix arguments would be necessary or appropriate.
What is your recommendation?

sbin/ipfw/ipfw.8

3837 ↗

(On Diff #156252)

My changes were to avoid confusion about the new vs old syntax. I will change or revert my patch on the ipfw manual, after we decide on the final syntax.

[des]

Just fix the segfault without changing the syntax: introduce iplen and eplen for the internal and external prefix lengths, and move the check (and the assignment to cfg->plen) out of the loop.

[glebius]

des's suggestion makes sense. It should be possible to support old syntax with printing a warning. The behavior should be:

• Both internal and external prefix lengths are specified with old syntax and are equal - use it, but print a warning
• Only one is specified with old syntax - use it, but print a warning
• Both are specified with old syntax and are different - fail to parse

Is it possible?

[p.mousavizadeh_protonmail.com]

Done

[des]

sbin/ipfw/nptv6.c

181

Why create a function for a trivial amount of code that will only be called once?

211

You only need to check this once, _after_ the loop.

[p.mousavizadeh_protonmail.com]

sbin/ipfw/nptv6.c

211

The user can specify arguments in any order, so I set the pplen (previous prefix length) in the nptv6_prefixlen_check function for each case to compare it with the prefix length values of the other arguments.
I could move the nptv6_prefixlen_check function to after the loop, but that would complicate the code by adding multiple if conditions.

Could you please help me understand how to implement your suggestion if my current approach is incorrect?

[p.mousavizadeh_protonmail.com]

sbin/ipfw/nptv6.c

181

This function runs for each argument related to prefix length. Therefore, it could run at most 3 times.

[des]

sbin/ipfw/nptv6.c

211

Check eplen, iplen, and pplen just once, after the loop.

[p.mousavizadeh_protonmail.com]

sbin/ipfw/nptv6.c

211

eplen, iplen, and plen have already been checked once.
eplen is checked only at line 237
iplen is checked only at line 227
plen is checked only at line 258

AFAIU, If I run my check after the loop, I won't be able to track pplen, so I'd have to repeat those checks multiple times.

[des]

sbin/ipfw/nptv6.c

211

Remove the calls to nptv6_prefixlen_check().

After the loop, check eplen, iplen, and pplen. If they are all zero, error out. If only one is non-zero, use that value. If more than one is non-zero, check that they all match, and either warn or error out if they don't.

[des]

sbin/ipfw/nptv6.c

236–237

Use pplen here.

[p.mousavizadeh_protonmail.com]

sbin/ipfw/nptv6.c

211

I have to compare 3x3 variables without separate function or list. Too complex to write, and probably much more complex to read. Here is why I did't want to do it after the loop.:

	if (iplen == 0) {
		if (eplen == 0) {
			if (plen == 0)
				errx(EX_USAGE, "prefixlen required");
			else
				pplen = plen;
		} else if (eplen == plen || plen == 0)
			pplen = eplen;
		else
			errx(EX_USAGE, "Prefix length mismatch (%d vs %d).",
				eplen, plen);
	} else {
		if (eplen == 0) {
			if (plen == 0)
				pplen = iplen;
			else if (iplen != plen)
				errx(EX_USAGE, "Prefix length mismatch (%d vs %d).",
					iplen, plen);
		} else if (iplen != eplen)
				errx(EX_USAGE, "Prefix length mismatch (%d vs %d).",
					iplen, eplen);
		else if (iplen != plen && plen != 0)
				errx(EX_USAGE, "Prefix length mismatch (%d vs %d).",
					iplen, plen);
		else
			pplen = iplen;
	}

[des]

sbin/ipfw/nptv6.c

211

No, you don't. First of all, there is no reason for plen to exist. Second, you don't need to print a detailed error message, just “prefix length mismatch” is enough. Third:

if (pplen != 0) {
    if ((eplen != 0 && eplen != pplen) || (iplen != 0 && iplen != pplen)
        errx(...);
    cfg->plen = pplen;
} else if (eplen != 0 || iplen != 0) {
    if (eplen != 0 && iplen != 0 && eplen != iplen)
        errx(...);
    warnx("use prefixlen instead");
    cfg->plen = eplen ? eplen : iplen;
} else {
    errx(...);
}

[p.mousavizadeh_protonmail.com]

Implement prefix length checking after the loop.

[p.mousavizadeh_protonmail.com]

sbin/ipfw/nptv6.c

211

1.plen was for prefixlen option; pplen was required by nptv6_prefixlen_check for tracking arguments.
As you requested, I replaced plen with pplen and removed plen and nptv6_prefixlen_check from the code.

2.I tried not to change the expected messages.
There is one problem with the new code:

# ipfw nptv6 test create int_prefix 2a01:e140:cafe:: ext_prefix 2a01:e140::
ipfw: Prefix length mismatch.

if prefixlen is not specified and int_prefix and ext_prefix don't include a prefix length, It says "prefix length mismatch.".
fixing this would add another condition. I could do that if you want.

[des]

sbin/ipfw/nptv6.c

211

You don't need an additional condition to fix this.

244–260

Move this (without the useless condition) to the two places where cfg->plen gets set.

247

247

You can drop this. Since cfg->plen remains zero, we hit “prefixlen required” below.

251

[des]

BTW, we generally don't capitalize error messages. It appears that ipfw sometimes does but mostly doesn't. You should probably avoid adding to the confusion.

[des]

sbin/ipfw/nptv6.c

239–240

drop this

[p.mousavizadeh_protonmail.com]

Remove useless condition

[p.mousavizadeh_protonmail.com]

Move hasprefix flag to where cfg->plen gets set.

[p.mousavizadeh_protonmail.com]

Thank you @des
Since this is a bugfix, It's better to commit it now rather than after the 15.0 release.
Is there anything I can help with or do to make that happen?

[des]

Sorry, I forgot you're not a committer. I'll handle it.

[des]

Unfortunately it looks like you uploaded a diff to your previous solution instead of a diff to main. Can you please update the review?

[p.mousavizadeh_protonmail.com]

Fix segfault on nptv6 and add a warning for not using the prefixlen option

[p.mousavizadeh_protonmail.com]

In D50597#1189768, @des wrote:

Sorry, I forgot you're not a committer. I'll handle it.

Looking for mentor to become one :)

Unfortunately it looks like you uploaded a diff to your previous solution instead of a diff to main. Can you please update the review?

Done, Thank you.