Repository navigation
Increase DNS response validations - #1370
kstribrnAmzn wants to merge 11 commits into
Conversation
7fdd341 to
226fb9a
Compare
| * of the DNS server. | ||
| * Later the received packet will be checked against this IP-address.*/ | ||
| memset( pxTargetAddress, 0, sizeof( *pxTargetAddress ) ); | ||
| xFamily = xAddress.sin_family; |
There was a problem hiding this comment.
Regression (all configs): query type now follows the DNS server's transport, not the caller's request.
xFamily is the requested record family and is passed to prvSendBuffer() (L1308), which picks dnsTYPE_AAAA_HOST vs dnsTYPE_A_HOST from it. Overwriting it with xAddress.sin_family means an AAAA lookup over an IPv4 DNS server sends an A query (and vice versa). Unrelated to the source-IP check and independent of ipconfigDNS_CHECK_REPLY_SOURCE_IP.
Suggest dropping this assignment and branching on xAddress.sin_family when filling pxTargetAddress. A test asserting the sent query type for getaddrinfo(AF_INET6) over an IPv4 server would catch it.
There was a problem hiding this comment.
Regression (all configs): query type now follows the DNS server's transport, not the caller's request.
xFamilyis the requested record family and is passed toprvSendBuffer()(L1308), which picksdnsTYPE_AAAA_HOSTvsdnsTYPE_A_HOSTfrom it. Overwriting it withxAddress.sin_familymeans an AAAA lookup
I think that sin_family determines the IP-protocol chosen to contact the DNS server. Will it look for either "8.8.8.8" or for e.g. "2001:4860:4860::8888".
It is still possible the retrieve a A record from an IPv6 server, and a AAA record from an IPv4 server.
There is a global variable called:
IPPreference_t xDNS_IP_Preference = xPreferenceIPv4;
It can have these values: None, IPv4 or IPv6.
When set (non zero), it indicates the preference for a DNS server type (IPv4 or IPv6).
In prvFillSockAddress(), the program is looking for a DNS address. Note that in this function sets pxAddress->sin_family, which is later used to prepare the UDP communication.
I found while testing, that the wrong type was addressed. I repaired it with this change:
/* of the DNS server.
* Later the received packet will be checked against this IP-address.*/
memset( pxTargetAddress, 0, sizeof( *pxTargetAddress ) );
+ xFamily = xAddress.sin_family;There was a problem hiding this comment.
The pxTargetAddress should now be filled from xAddress.sin_family, addressing your concern.
| * answer record's owner name matches what was asked. | ||
| * Please check if enough stack memory is available. */ | ||
| char pcNameAnswer[ ipconfigDNS_CACHE_NAME_LENGTH ]; | ||
| ( void ) strncpy( pcNameAnswer, pxSet->pcName, sizeof pcNameAnswer ); |
There was a problem hiding this comment.
The name check validates the reply against itself, not against what was queried.
pcNameAnswer is snapshotted from pxSet->pcName, which DNS_ParseDNSReply() filled from the received packet's question section (or first answer when QDCOUNT==0). Nothing compares it with pcHostName sent in prvGetHostByNameOp() or pxCallback->pcName; xDNSDoCallback() matches by transaction ID only (except mDNS).
So a forged reply with the right ID, question = victim.example, answer owner = 0xC00C, A = attacker IP passes this check and is cached via FreeRTOS_dns_update( pxSet->pcName, ... ).
Suggest passing the expected hostname into DNS_ParseDNSReply() and rejecting when the question name isn't strcasecmp-equal to it (and comparing against pxCallback->pcName on the async path).
There was a problem hiding this comment.
The name check ( what we're looking for and what we find ) has been re-visioned. It looks good to me and I have tested it by supplying the spoofed IP-address and/or a wrong host "unhappy.com" in stead of "happy.com".
There was a problem hiding this comment.
A requested name is now provided to DNS_ParseDNSReply
| * unchanged for a compressed name and would make the | ||
| * comparison pass without validating anything. */ | ||
| if( ( prvReadAnswerName( pxSet, pcDecodedAnswer, sizeof pcDecodedAnswer ) == pdFALSE ) || | ||
| ( strcasecmp( pcDecodedAnswer, pcNameAnswer ) != 0 ) ) |
There was a problem hiding this comment.
Breaks CNAME-chained resolution in the default config.
For a typical reply www.example.com CNAME cdn.example.net / cdn.example.net A 1.2.3.4, the A record's owner is the CNAME target, so this mismatch sets uxResult = 0 → xReturn = pdFALSE; break; and the whole response is dropped. This check isn't gated by ipconfigDNS_CHECK_REPLY_SOURCE_IP and is active whenever DNS cache/callbacks/mDNS/LLMNR is enabled, so most CDN-backed hostnames would stop resolving. Multi-owner mDNS responses (PTR/SRV/TXT/A for different names) hit the same path.
Suggest accepting an owner equal to the queried name or a CNAME target already seen in this response, or skipping non-matching records rather than failing the response. A CNAME-chain unit test would be good to add.
Related: when this fires mid-loop, records 1..N-1 are already in the DNS cache / *ppxAddressInfo, and the post-loop callback (L1135) still runs. Validate-then-commit would avoid acting on a response that's reported as invalid.
There was a problem hiding this comment.
It looks good to me. These changed were also tested with a Python script.
Thanks
There was a problem hiding this comment.
Mismatches now skip instead of fail.
| xDoRead = pdTRUE; | ||
| } | ||
|
|
||
| if( ( xDoRead == pdTRUE ) && ( DNS_ReadNameField( &( xNameSet ), uxDestLen ) != 0U ) ) |
There was a problem hiding this comment.
Bypass: pointer-to-pointer owner name passes vacuously.
xNameSet is a memcpy of *pxSet, so xNameSet.pcName starts as the queried name. If the outer pointer targets another pointer (e.g. C0 xx pointing at itself or any other C0 ..), DNS_ReadNameField() takes its offset branch, returns 2 and writes nothing; the strlen/memcpy below then copies the stale queried name and the comparison passes — the exact failure mode the doc comment says this helper avoids.
Also, DNS_ReadNameField() doesn't handle labels followed by a pointer (03 'c' 'd' 'n' C0 10), so legitimate names in that form are rejected.
Suggest a dedicated decoder/comparator over (message base, length, offset) with a hop limit (or backward-only pointers), that never seeds output from caller state. Clear xNameSet.pcName[0] at minimum. Worth a unit test for C0 xx → C0 yy.
Stack note: this function plus pcNameAnswer/pcDecodedAnswer add roughly 800 B (2×254 + a full ParseSet_t copy) on the IP-task path (xDNSHandlePacket).
There was a problem hiding this comment.
Thanks for mentioning the stack usage. I also keep on monitoring it.
Thanks
There was a problem hiding this comment.
DNS_DecodeName replaces the old helper. DNS_ReadNameField has been updated to handle labels followed by a pointer.
| { | ||
| pCallbackFunction( pxSet->pcName, pvSearchID, pxAddress ); | ||
| } | ||
| *( ppvSearchID ) = pvSearchID; |
There was a problem hiding this comment.
pvSearchID is clobbered to NULL by subsequent records.
parseDNSAnswer() still calls xDNSDoCallback() once per accepted A/AAAA record, and this line writes *ppvSearchID unconditionally. Record 1 matches (entry removed, ID set); record 2 finds nothing and writes NULL. xCallbackEvent is latched by the caller but pvSearchID isn't, so the user callback receives pvSearchID == NULL for any multi-A reply — likely a NULL deref in apps that pass a context pointer.
Suggest writing *ppvSearchID only on match, or calling xDNSDoCallback() once outside the loop. test_parseDNSAnswer_two_records_stored_with_callback doesn't assert the ID received, so it can't catch this.
There was a problem hiding this comment.
Moved the assignment of ppvSearchID
| uint32_t ulAddressLength = ( uint32_t ) sizeof( struct freertos_sockaddr ); | ||
| struct freertos_sockaddr xFromAddress; | ||
|
|
||
| TickType_t xTimeoutTime = xIsCallingFromIPTask() ? 0u : pdMS_TO_TICKS( 500u ); |
There was a problem hiding this comment.
Hard-coded 500 ms RCVTIMEO overrides config and async mode.
DNS_CreateSocket()already set the timeout touxReadTimeOut_ticks; forFreeRTOS_gethostbyname_a()that's 0 (non-blocking), but this now blocks the calling task 500 ms.- Sync callers lose
ipconfigDNS_RECEIVE_BLOCK_TIME_TICKS(default 5 s) → slower links time out, and each timeout rotates the DNS server index. - Return value of
FreeRTOS_setsockopt()is ignored (MISRA 17.7), and it's why the prototype droppedConstSocket_t.
This seems unrelated to reply validation; could it be removed or split into its own change?
| } | ||
| else | ||
| { | ||
| xMatch = ( pxTargetAddress->xIPAddress.ulIP_IPv4 == ipMDNS_IP_ADDRESS ) ? pdTRUE : pdFALSE; |
There was a problem hiding this comment.
With ipconfigDNS_CHECK_REPLY_SOURCE_IP == 1 and ipconfigUSE_LLMNR == 1, LLMNR queries go to ipLLMNR_IP_ADDR / ipLLMNR_IP_ADDR_IPv6 but responses are unicast from the responder, so they'll all be rejected here. Suggest exempting any multicast target (mDNS + LLMNR, v4 + v6) rather than mDNS only.
There was a problem hiding this comment.
prvIsMulticastTarget() now exempts any multicast target
| FreeRTOS_debug_printf( ( "DNS_ReadReply: Source mismatch, discarding packet.\n" ) ); | ||
|
|
||
| /* Return an error so the caller knows this packet is invalid */ | ||
| xReturn = -pdFREERTOS_ERRNO_EINVAL; |
There was a problem hiding this comment.
Returning -EINVAL ends the attempt: prvGetHostByNameOp() does one recvfrom per send, so one injected wrong-source packet consumes an attempt (and 5 kill the lookup). Pre-existing for ID mismatches, but this adds a new trigger. Consider discarding the packet and continuing to read until the original deadline.
There was a problem hiding this comment.
We can add this later. I don't see how it is critical right now (this is already a large PR).
| */ | ||
|
|
||
| #ifndef ipconfigDNS_CHECK_REPLY_SOURCE_IP | ||
| #define ipconfigDNS_CHECK_REPLY_SOURCE_IP ipconfigDISABLE |
There was a problem hiding this comment.
Coverage gap worth documenting or fixing: this check lives only in DNS_ReadReply(). Replies that arrive after the socket is closed (the normal case for FreeRTOS_gethostbyname_a() / callback mode) are handled by xDNSHandlePacket() → DNS_ParseDNSReply() with no source check, so enabling this option doesn't protect the callback path. Storing the queried server in the DNSCallback_t entry and checking it there would close it.
There was a problem hiding this comment.
There was a problem hiding this comment.
Coverage gap worth documenting or fixing: this check lives only in
DNS_ReadReply(). Replies that arrive after the socket is closed (the normal case forFreeRTOS_gethostbyname_a()/ callback mode) are handled byxDNSHandlePacket()→DNS_ParseDNSReply()with no source check, so enabling this option doesn't protect the callback path. Storing the queried server in theDNSCallback_tentry and checking it there would close it.
I don't see that as very likely. All UDP packets will be tested on both the IP-address and hostname to lookup.
Once the UDP socket is closed, it can not receive any packets anymore.
All DNS replies go through parseDNSAnswer(), also the asynchronous lookup's.
When there is time I wouldn't mind to setting up a live test.
Cheers,
Hein
There was a problem hiding this comment.
Yeah - maybe we can add this later. What are your thoughts @htibosch?
fb21550 to
42e1aac
Compare
Verifies the DNS reply matches against the server which was queried. We thank Causal Security (https://causalsecurity.com/) for their help identifying this issue. We thank @htibosch for the extensive help developing source code changes for this fix
Answer-record owner names Validate each answer record's owner name against the name being resolved, following CNAME records so the address records behind an alias are still accepted. A record owned by some other name is now skipped instead of invalidating the whole response, because a response may legally carry records for names that were not asked for. Note that the comparison is against the question name carried in the reply itself, not against the host name that the caller passed to FreeRTOS_gethostbyname() or FreeRTOS_getaddrinfo(). Nothing yet binds a reply to the name that was requested beyond its transaction ID, and this change does not add that binding. The owner name is decoded by DNS_DecodeName(), which resolves compression pointers against the start of the message, handles a sequence of labels that ends in a pointer, bounds the number of pointers it follows, and never seeds its output from caller state. The previous helper started from a copy of the queried name, so an owner name that pointed at another pointer decoded nothing and compared equal. DNS_SkipNameField() now also steps over a name that ends in a pointer. Query type Stop overwriting 'xFamily' with the DNS server's address family. 'xFamily' is the type of record being asked for; the two are independent, and an AAAA record may well be requested from a DNS server that is reached over IPv4. Asynchronous search ID xDNSDoCallback() now reports the search ID only when it matched a request. It is called once per address record and removes the entry on the first match, so writing unconditionally cleared the ID again for any reply that carries more than one address. Receive time-out Drop the hard-coded 500 ms SO_RCVTIMEO from DNS_ReadReply(). The socket's time-out is set when the socket is created, from ipconfigDNS_RECEIVE_BLOCK_TIME_TICKS for a blocking look-up and from zero for an asynchronous one. Multicast replies Exempt any multicast query target from the source-IP check rather than only the mDNS addresses, so that LLMNR replies are not rejected when ipconfigDNS_CHECK_REPLY_SOURCE_IP is enabled. Also document that ipconfigDNS_CHECK_REPLY_SOURCE_IP does not cover the asynchronous look-up path, remove the development logging, and add the missing MISRA 16.4 and 17.7 conformance.
All four DNS_ParseDNSReply configurations failed with an unwinding assertion on DNS_DecodeName.0. The decoder was reached from DNS_ParseDNSReply() on the path where a response carries no question and the name has to be taken from the first answer instead, and that loop is not in those proofs' unwindsets, so --unwind 1 applied to it. Take the name with DNS_ReadNameField() there instead of DNS_DecodeName(). The decoder was only used to reach it under the same condition that the answer-name validation is compiled under, and widening the declaration of DNS_ReadNameField() to the condition its definition already uses achieves that without adding a loop to this entry point. Nothing precedes those records but the header, so the name cannot usefully be a compression pointer; should it be one anyway, the name stays empty and every answer record fails the validation. Bounding DNS_DecodeName.0 in those proofs instead would have worked, but it made the two NetworkBufferSize configurations far more expensive: the IPv4 dynamic one exhausted the SAT solver's memory locally. With this change the four proofs take 1s, 1s, 87s and 108s and all report SUCCESS.
b505f55 to
64023d6
Compare
Every name in a reply is supplied by whoever sent it. A reply was matched to an outstanding look-up by its transaction ID alone, and the owner-name validation then compared the answer records against the question section of that same reply, so a reply that was self-consistent about some other name was accepted on the strength of the identifier. Its addresses were returned to the caller that had asked for a different name, and cached under the name the reply carried. Both look-up paths now check the reply against the name this device asked for: - DNS_ParseDNSReply() takes the requested name and rejects a response whose question section is about anything else. prvDNSReply() passes the host name down from prvGetHostByNameOp(), which has it. - xDNSDoCallback() requires a reply to carry both the identifier of the request and its name before handing it to that request's call-back. Previously only mDNS compared the name. This is what binds the asynchronous path, where the reply is parsed by the IP task: xDNSHandlePacket() passes NULL for the requested name because it does not know which look-up a packet belongs to. This does not add a source-address check to the asynchronous path, which is a separate concern tracked on its own.
Keep only what a reader would otherwise mispredict - a hidden constraint, a workaround, or a deliberate non-obvious choice - and hold each of those to one sentence. What survives, and why: - 'xFamily' is the record type, not the server's address family, so it must not be overwritten. Removing this invites the regression back. - The receive time-out belongs to the socket, so DNS_ReadReply() must not overrule it. - DNS_SkipNameField() steps over a name that ends in a compression pointer, which therefore has no terminating zero octet. - DNS_DecodeName() must never seed its output from caller state, and leaves no partial name behind; dnsMAX_COMPRESSION_HOPS is overridable only so the CBMC proof of its loop stays tractable. - parseDNSAnswer() skips the owner name rather than reading it, so that the cache and call-backs stay keyed on the queried name; a record owned by another name is skipped rather than failing the response; a CNAME target that cannot be decoded rejects every later record. - xDNSDoCallback() is called once per address record, so it reports the search ID only on a match, and an identifier alone does not identify a reply. - xDNSHandlePacket() passes no requested name because the IP task does not know it. Doxygen blocks on the new functions are kept because EXTRACT_STATIC and WARN_IF_UNDOCUMENTED are both set, but are cut to a one-sentence brief.
The doxygen job fails on an undocumented parameter.
| { | ||
| /* After the loop, xCallbackEvent will be called | ||
| * as a function. */ | ||
| xCallbackEvent = xNewEvent; |
There was a problem hiding this comment.
Regression: two pending async look-ups for the same name get cross-wired.
xDNSDoCallback() is still called once per accepted address record, and each call removes and frees the matching list entry. Here only the first returned callback is kept, but pvSearchID is overwritten on every match.
Trace: two FreeRTOS_gethostbyname_a( "printer.local", ... ) requests are pending (mDNS matches on name only), and the reply carries an A and an AAAA record.
- Record 1 matches entry A, so
xCallbackEvent = A.fnandpvSearchID = A.id. Entry A is freed. - Record 2 matches entry B, so
xNewEvent = B.fnis discarded andpvSearchID = B.id. Entry B is freed. - After the loop,
A.fn( name, B.id, list )runs.
B's callback never runs, and because its entry is already freed, vDNSCheckCallBack() can't fire its timeout either. Before this change each match invoked its own callback.
Suggestion: the match depends only on the header ID and pxSet->pcName, which are both known before the answer loop. Call xDNSDoCallback() once before the loop and invoke the returned callback once after it. Alternatively, keep each (function, search ID) pair together and invoke every pair. A unit test with two pending same-name requests would pin this.
| { | ||
| /* Every name in a reply is supplied by whoever sent | ||
| * it, so the identifier alone proves nothing. */ | ||
| if( strcasecmp( xSet.pcName, pcRequestedName ) != 0 ) |
There was a problem hiding this comment.
Regression: names with a trailing dot no longer resolve.
prvCreateDNSMessage() encodes "example.com." exactly like "example.com" on the wire (7example3com0). The reply's question therefore decodes to "example.com", and this strcasecmp() against "example.com." fails, so every reply is rejected after all retries. The async path fails the same way: the new strcasecmp( pxCallback->pcName, pxSet->pcName ) in xDNSDoCallback() (FreeRTOS_DNS_Callback.c:92) never matches, and the request only times out.
These look-ups worked before this PR.
Suggestion: ignore a single trailing . on either side before comparing, for example with a small name-equality helper used here and in xDNSDoCallback(). A small ASCII helper would also avoid making POSIX strcasecmp a dependency of the default build; it used to be used only under ipconfigUSE_MDNS.
There was a problem hiding this comment.
Fair suggestion. I don't want to be dropping FQDNs.
| { | ||
| FreeRTOS_printf( ( "DNS: could not decode the name of answer %u\n", | ||
| ( unsigned ) x ) ); | ||
| uxResult = 0U; |
There was a problem hiding this comment.
Regression: one answer owner name that can't be decoded rejects the whole reply.
Setting uxResult = 0U here takes the "malformed response" branch below (xReturn = pdFALSE; break;), which discards every answer, including addresses already collected. DNS_DecodeName() returns pdFALSE not only for malformed input but also when:
- the name doesn't fit
ipconfigDNS_CACHE_NAME_LENGTH, or - the name needs more than
dnsMAX_COMPRESSION_HOPSpointer hops.
Before this PR, answer owner names were only skipped, so their length didn't matter. A short name whose CNAME chain passes through a long CDN name, or any device with a lowered ipconfigDNS_CACHE_NAME_LENGTH, now fails to resolve. It also contradicts the intent stated a few lines below, that a record owned by another name is skipped rather than failing the response.
Suggestion: since DNS_SkipNameField() already succeeded, treat a decode failure as a mismatch so the record is skipped by RDLENGTH:
if( prvReadCurrentName( pxSet, pcDecodedAnswer, sizeof pcDecodedAnswer ) == pdFALSE )
{
xNameMatch = pdFALSE;
}Keep the hard failure for DNS_SkipNameField() == 0.
This branch has not been deployed
3 participants
Description
Verifies the DNS reply matches against the
server which was queried.
We thank Causal Security (https://causalsecurity.com/) for their help identifying this issue.
We thank @htibosch for the extensive help developing source code changes for this fix
Test Steps
Added unit tests
Checklist:
Related Issue
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.