$Id: compliance-nntp 6817 2004-05-18 09:25:55Z rra $ The following are outstanding issues regarding INN's compliance with the NNTP standard. The reference documents used in this analysis are the current NNTP IETF Working Group draft (draft-ietf-nntpext-base-15.txt at the time of the last check of this audit) or RFC 2980, not RFC 977 (which is woefully out of date). This file documents only compliance issues with the latest version of the standard NNTP protocol. It does not cover INN's private extensions or INN's implementation of widely available extensions not documented in the NNTP standard. Specifically, it does not cover the extensions listed in RFC 2980. ------------------------------ Summary: innd doesn't require whitespace between commands and arguments Standard: draft-ietf-nntpext-base-15.txt, section 4 Version: 1.0 to CURRENT 2002-12-26 Reference: innd/nc.c NCproc() and command handlers Severity: Accepts invalid input The standard states: Keywords and arguments MUST be each separated by one or more US-ASCII SPACE or US-ASCII TAB characters. This is not checked in NCproc or in the individual command handlers in innd. Commands followed immediately by their argument will be accepted by innd. For example: stat<9k6vjk.hg0@example.com> 223 0 @0301543531000000000000079AAE0000006A@ Impact: Should one command be a prefix of another, innd could dispatch the handling of the command to the wrong handler, treating the remainder of the command verb as an argument. This laxness also encourages sloppy client code. Internally, the lack of argument parsing in NCproc also results in code duplication in all of the command handlers. Suggested fix: Lift the argument parsing code into a function called from NCproc, breaking the command line into a vector of command and arguments. This will work for all commands implemented by innd and will simplify the implementation of command handlers, as well as fixing this problem. This is what nnrpd already does. Impact of fix: It's possible that some serving code is relying on this behavior and not sending spaces after commands. Fixing this problem would break interoperability with that code. ------------------------------ Summary: INN doesn't check argument length Standard: draft-ietf-nntpext-base-15.txt, section 4 Version: 1.0 to CURRENT 2002-12-26 Reference: innd/nc.c and nnrpd/nnrpd.c Severity: Accepts invalid input The standard says: Arguments MUST NOT exceed 497 octets. This is not checked by either innd or nnrpd, although both do check that the command itself does not exceed 512 octets. Impact: Small. May accept invalid commands in extremely rare edge cases. Suggested fix: Probably not worth fixing separately, although if standard command parsing code is written to handle both innd and nnrpd, it wouldn't hurt to check this along with everything else. ------------------------------ Summary: Reply codes other than x9x used for private extensions Standard: draft-ietf-nntpext-base-15.txt, section 4.1 Version: 1.0 to CURRENT 2002-12-26 Reference: include/nntp.h Severity: Violates SHOULD The standard says: Response codes not specified in this standard MAY be used for any installation-specific additional commands also not specified. These SHOULD be chosen to fit the pattern of x9x specified above. INN uses quite a few response codes that do not fit this pattern for various extensions. Some of these will likely later be standardized with the response codes that INN uses (the streaming commands, the authentication reply codes, and possibly the STARTTLS reply codes), but the rest (XGTITLE, MODE CANCEL, and XBATCH) should have used response codes in the x9x range. Impact: Additional ambiguity over the meaning of reply codes, as those reply codes could later be standardized as the reply codes for other commands. Suggested fix: For XGTITLE and probably XBATCH, there is no way to fix this now. Changing the reply codes would break all existing implementations. It may still be possible to change the reply codes for MODE CANCEL (which should probably be MODE XCANCEL), but it may not be worth the effort. ------------------------------ Summary: innd may return 480 instead of 500 for unrecognized commands Standard: draft-ietf-nntpext-base-15.txt, section 4.1.1 Version: 1.0 to CURRENT 2002-12-26 Reference: innd/nc.c NCauthinfo() Severity: Violates MUST The standard says: If the command is not recognized, or it is an optional command or extension that is not implemented by the server, the response code 500 MUST be returned. In innd, if the connection is determined to need authentication, all incoming commands other than MODE are handed off to NCauthinfo() rather than their normal command handlers. NCauthinfo() responds with a 480 reply code to anything other than AUTHINFO USER, AUTHINFO PASS, or QUIT. Impact: Unlikely to cause problems in practice, but may confuse clients that don't understand the rarely used innd-level authentication mechanisms. Suggested fix: Restructure the command table so that each command also has a flag indicating whether it requires authentication for peers that are required to authenticate. (Some commands, like HELP and MODE READER, should be allowed without authentication.) Then eliminate the special casing of the state CSgetauth (it may be better to store whether the peer has authenticated in the channel rather than in the channel state) and the special handling in NCauthinfo. This should also simplify the code. ------------------------------ Summary: innd always sends 200 for an accepted connection Standard: draft-ietf-nntpext-base-15.txt, section 7.1 Version: 1.0 to CURRENT 2002-12-26 Reference: innd/nc.c NCsetup() and rc.c RCreader() Severity: Violates MUST The standard says: If the server will accept further commands from the client including POST, the server MUST present a 200 greeting code. If the server will accept further commands from the client, but it is not authorized to post articles using the POST command, the server MUST present a 201 greeting code. The implication is that the greeting code from innd (which doesn't implement POST and therefore is never going to allow it) should always be 201, at least for the case where innd never spawns nnrpd. In the case where innd spawns nnrpd, it's unclear what the greeting code should be. The current implementation nevers send 201 unless one knows for certain that the connection will never be allowed to issue a POST command, which means that innd always sends 200. It's unknown whether there is any transit news software that would have difficulties with a 201 greeting. Both innxmit and innfeed handle it correctly in CURRENT 2001-07-04 and NNTPconnect() handles it correctly in INN 1.0, so it seems likely that if any such software exists, it's rare. Impact: It's almost certain that the current innd behavior isn't hurting anything. Even a confused client that thought 200 meant that it could send a POST command would then try and be rejected with no damage done. Suggested fix: The purpose of this return code is to give a hint to a reading client indicating whether it should even attempt POST, since attempting it may involve a lot of work by the user only to have the post rejected. It's only relevant to reading connections, not to transit connections. It's known that some clients, upon seeing a 201 response, will never attempt POST, even if MODE READER then returns 200. Therefore innd, when handing off connections to nnrpd, must return 200 to not confuse a client that will later send MODE READER. For connections where innd won't do that handoff, it makes sense to always send 201 if all transit feeds can handle that and won't interpret it as unwillingness to accept IHAVE or streaming feeds. RCreader() should therefore be modified to send 201 if noreader is set, and otherwise send 200. Impact of fix: Any feeding software that didn't consider 201 to be a valid greeting would be unable to feed a fixed innd unless that innd also allowed reading connections. ------------------------------ Summary: innd doesn't support LIST EXTENSIONS Standard: draft-ietf-nntpext-base-15.txt, section 8.1 Version: 1.0 to CURRENT 2002-12-26 Reference: innd/nc.c NClist() Severity: Not a violation Support for LIST EXTENSIONS is optional, and innd's current behavior (returning a 500 response) is permitted by the standard, but it means that innd cannot advertise any of the extensions that it supports. Since this will eventually include streaming, support should be added. Suggested fix: Add support for LIST EXTENSIONS to NClist() as soon as innd supports a registered extension or as soon as there is documentation for INN's extensions that specify an extension name (beginning with X). ------------------------------ Summary: nnrpd doesn't return 423 errors when there is no overview info Standard: draft-ietf-nntpext-base-17.txt, section 10.5.1.2 Version: 1.4 to CURRENT 2003-05-04 Reference: nnrpd/article.c CMDxover() Severity: Violates a MUST The standard says: If there are no articles in the range specified, a 423 response MUST be returned. nnrpd (from the beginning of the XOVER command) has always returned a 224 response with an empty multiline response instead. INN doesn't support OVER yet so this isn't actually a bug in INN, but eventually the XOVER implementation will also be used for OVER. Impact: Less information is communicated to the client about why there are no overview records returned. An error response indicating there are no valid articles in that range is possibly more informative. Suggested fix: Don't print out the initial 224 message until at least one overview entry has been found, so that CMDxover() can print a 420 response instead if no overview records are found. Impact of fix: May confuse some clients that don't expect to get 420 errors back from overview queries. It may be necessary to do something different for OVER (where clients should expect this behavior since OVER is a new command) than for XOVER (where clients may be relying on the existing behavior. ------------------------------ Summary: HDR can return message IDs rather than article numbers Standard: draft-ietf-nntpext-base-17.txt, section 10.6.1.2 Version: 1.0 to CURRENT 2003-05-04 Reference: nnrpd/article.c CMDpat() Severity: Violates a protocol description The standard says: The line consists of the article number, a US-ASCII space, and then the contents of the header (without the header name or the colon and space that follow it) or metadata item. If the article is specified by message-id rather than by article range, the article number is given as "0". nnrpd instead returns the message ID as the first word of the line when HDR is given a message ID argument. Impact: A client may not be able to parse the output of HDR correctly, since the first word is not a number. Suggested fix: Change the code to return 0 as the first word instead of the message ID, per the standard. Impact of fix: Clients that are expecting the message ID may be confused. ------------------------------ Summary: innd improperly caches DNS returns Standard: draft-ietf-nntpext-base-15.txt, section 14.4 Version: 1.0 to CURRENT 2002-12-26 Reference: innd/rc.c RCreadfile() and elsewhere Severity: Violates a MUST The standard says: If NNTP clients or servers cache the results of host name lookups in order to achieve a performance improvement, they MUST observe the TTL information reported by DNS. innd caches DNS lookups when reading incoming.conf and doesn't refresh its knowledge of DNS except when incoming.conf is reloaded. Impact: An explicit reload is required whenever the IP address of any peer changes, and in the presence of network renumbering innd is vulnerable to spoofing if DNS is the only authentication mechanism used. Suggested fix: This is hard to fix without unacceptable performance impact. The only good fix is to either fork a separate helper process to do DNS lookups (since gethostbyname may block for essentially an arbitrarily long period) or to use the direct resolver library so that one can get access to a file descriptor and throw it into the select loop. Either way, this requires keeping a DNS file descriptor in the main select loop and updating knowledge of DNS periodically, which is a substantial amount of additional complexity. ------------------------------ Summary: innd doesn't diagnose repeated AUTHINFO USER commands Standard: RFC 2980, section 3.1.1 Version: 1.0 to CURRENT 2002-12-26 Reference: innd/nc.c NCauthinfo() Severity: Violates a protocol description RFC 2980 says: The 482 code will also be returned when the AUTHINFO commands are not entered in the correct sequence (like two AUTHINFO USERs in a row, or AUTHINFO PASS preceding AUTHINFO USER). innd ignores AUTHINFO USER and just always returns a 381 response, however, since it doesn't care about the username. Impact: Probably none. Suggested fix: A long-term solution would be to add real authentication to innd, in which case it would start caring about the authenticated identity (and perhaps use that identity to map to an incoming.conf entry). It's unclear if this would be worthwhile. Failing that, innd would need to keep internal state to know whether AUTHINFO USER had already been sent.