netdev - Re: v3 [PATCH 1/1] dccp: Process incoming Change feature-negotiation options

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20080903082409.GB9001@gerrit.erg.abdn.ac.uk>
Date:	Wed, 3 Sep 2008 10:24:09 +0200
From:	Gerrit Renker <gerrit@....abdn.ac.uk>
To:	Wei Yongjun <yjwei@...fujitsu.com>, dccp@...r.kernel.org,
	netdev@...r.kernel.org
Subject: Re: v3 [PATCH 1/1] dccp: Process incoming Change
	feature-negotiation options

| >> +static u8 dccp_feat_change_recv(struct list_head *fn, u8 is_mandatory, u8 opt,
| >> +				u8 feat, u8 *val, u8 len, const bool server)
<snip>
| >> +	if (type == FEAT_NN) {
| >> +		if (local)
| >> +			goto not_valid_or_not_known;
| >>   
| > ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
| >
| >  Here should be "goto unknown_feature_or_value"?
| >
| > Change R is invalid, and RFC4340 6.6.8
| >
| > An endpoint receiving an invalid Change option MUST respond with the
| > corresponding empty Confirm option.
| >
| > Is this right?
| I am not sure.
| Section 6.3.2 says that Change R options must not be sent for NN options. A
| Change R is different from an invalid value - it is an option which is out
| of place in such a context. A sender using such an option for NN features
| has a clear bug.
| 
| Hence my interpretation of this situation was to flag this bug to the peer:
|  * the peer gets sent an Option Error rather than an empty Confirm,
|  * if it was a Mandatory option, both interpretations are equivalent.
| 
| But there is also the robustness principle (3.6), so thank you, I will
| change it to the `unknown_feature_or_value'.
| 
I have serious doubts whether the above is the right thing. Jumping to
unknown_feature_or_value means the following

unknown_feature_or_value:
        if (!is_mandatory)
		return dccp_push_empty_confirm(fn, feat, local);

not_valid_or_not_known:
        return is_mandatory ? DCCP_RESET_CODE_MANDATORY_ERROR
	                    : DCCP_RESET_CODE_OPTION_ERROR;

==> Hence the host is asked to send an invalid option type, a Confirm L,
    in response to the invalid Change R.
    A Confirm L in the context of negotiating NN features is never a valid
    option, so this jump is paradox. I think this was the reason why it
    was originally a Reset. 

    Are there any objections reverting this to the original state, i.e.
    sending a Reset instead of a Confirm L for an out-of-place Change R?

Gerrit
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html