From patchwork Fri Sep 11 13:05:17 2020 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Florian Weimer X-Patchwork-Id: 1362432 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Authentication-Results: ozlabs.org; spf=pass (sender SPF authorized) smtp.mailfrom=sourceware.org (client-ip=8.43.85.97; helo=sourceware.org; envelope-from=libc-alpha-bounces@sourceware.org; receiver=) Authentication-Results: ozlabs.org; dmarc=pass (p=none dis=none) header.from=sourceware.org Authentication-Results: ozlabs.org; dkim=pass (1024-bit key; secure) header.d=sourceware.org header.i=@sourceware.org header.a=rsa-sha256 header.s=default header.b=g4kCOoNC; dkim-atps=neutral Received: from sourceware.org (server2.sourceware.org [8.43.85.97]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (4096 bits) server-digest SHA256) (No client certificate requested) by ozlabs.org (Postfix) with ESMTPS id 4Bnwwb2C2Tz9ryj for ; Fri, 11 Sep 2020 23:05:35 +1000 (AEST) Received: from server2.sourceware.org (localhost [IPv6:::1]) by sourceware.org (Postfix) with ESMTP id 2B7523987463; Fri, 11 Sep 2020 13:05:33 +0000 (GMT) DKIM-Filter: OpenDKIM Filter v2.11.0 sourceware.org 2B7523987463 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=sourceware.org; s=default; t=1599829533; bh=LI8YLerTjmAWP8LwBE5ZoZP5jl0Z/2UA40WeAbMdVVk=; h=To:Subject:In-Reply-To:References:Date:List-Id:List-Unsubscribe: List-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To: From; b=g4kCOoNCpjfbFcq3OQKYEB+Rfch6NExLZ9dMob06FVJDTIhHzuV5Nk/Hnzq4ZqJXv wqwnSAO0WE7B66pMYaqvXgpzLy2CsfQ7HNUusMsO1UH9PK5p78lZseVsIu+FkljYFP SOABNeSrSfjpyqpWjtH+KcGpPG1HkqCB2eIKTQ2g= X-Original-To: libc-alpha@sourceware.org Delivered-To: libc-alpha@sourceware.org Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [216.205.24.124]) by sourceware.org (Postfix) with ESMTP id 8DE64398742D for ; Fri, 11 Sep 2020 13:05:29 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org 8DE64398742D Received: from mimecast-mx01.redhat.com (mimecast-mx01.redhat.com [209.132.183.4]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-595-dk-BMNt2PaGhJQbF-RkfUw-1; Fri, 11 Sep 2020 09:05:21 -0400 X-MC-Unique: dk-BMNt2PaGhJQbF-RkfUw-1 Received: from smtp.corp.redhat.com (int-mx07.intmail.prod.int.phx2.redhat.com [10.5.11.22]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id D369C56BE8 for ; Fri, 11 Sep 2020 13:05:20 +0000 (UTC) Received: from oldenburg2.str.redhat.com (ovpn-113-142.rdu2.redhat.com [10.10.113.142]) by smtp.corp.redhat.com (Postfix) with ESMTPS id 747D51002D6C for ; Fri, 11 Sep 2020 13:05:19 +0000 (UTC) To: libc-alpha@sourceware.org Subject: [PATCH 3/3] resolv: Handle transaction ID collisions in parallel queries (bug 26600) In-Reply-To: <55f9a2d387df8fae7d44275e2cf7dce8eccd120e.1599829382.git.fweimer@redhat.com> References: <55f9a2d387df8fae7d44275e2cf7dce8eccd120e.1599829382.git.fweimer@redhat.com> Date: Fri, 11 Sep 2020 15:05:17 +0200 Message-ID: <87lfhgqx9u.fsf@oldenburg2.str.redhat.com> User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/27.1 (gnu/linux) MIME-Version: 1.0 X-Scanned-By: MIMEDefang 2.84 on 10.5.11.22 X-Mimecast-Spam-Score: 0.002 X-Mimecast-Originator: redhat.com X-Spam-Status: No, score=-11.6 required=5.0 tests=BAYES_00, DKIMWL_WL_HIGH, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, GIT_PATCH_0, KAM_SHORT, RCVD_IN_DNSWL_NONE, RCVD_IN_MSPIKE_H5, RCVD_IN_MSPIKE_WL, SPF_HELO_NONE, SPF_PASS, TXREP autolearn=ham autolearn_force=no version=3.4.2 X-Spam-Checker-Version: SpamAssassin 3.4.2 (2018-09-13) on server2.sourceware.org X-BeenThere: libc-alpha@sourceware.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Libc-alpha mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-Patchwork-Original-From: Florian Weimer via Libc-alpha From: Florian Weimer Reply-To: Florian Weimer Errors-To: libc-alpha-bounces@sourceware.org Sender: "Libc-alpha" If the transaction IDs are equal, the old check attributed both responses to the first query, not recognizing the second response. This fixes bug 26600. (Tested on x86_64-linux-gnu. I verified that the referral path for RCODE 0 responses is taken. I also linked tst-resolv-txnid-collision.o against an ABI-compatible static glibc build without the fix, to verify that the test reproduces the bug.) --- resolv/Makefile | 7 + resolv/res_send.c | 40 ++-- resolv/tst-resolv-txnid-collision.c | 329 ++++++++++++++++++++++++++++ 3 files changed, 356 insertions(+), 20 deletions(-) create mode 100644 resolv/tst-resolv-txnid-collision.c diff --git a/resolv/Makefile b/resolv/Makefile index b61c0c3e0c..dbd8f8bf4f 100644 --- a/resolv/Makefile +++ b/resolv/Makefile @@ -61,6 +61,11 @@ tests += \ tst-resolv-search \ tst-resolv-trailing \ +# This test calls __res_context_send directly, which is not exported +# from libresolv. +tests-internal += tst-resolv-txnid-collision +tests-static += tst-resolv-txnid-collision + # These tests need libdl. ifeq (yes,$(build-shared)) tests += \ @@ -191,6 +196,8 @@ $(objpfx)tst-resolv-search: $(objpfx)libresolv.so $(shared-thread-library) $(objpfx)tst-resolv-trailing: $(objpfx)libresolv.so $(shared-thread-library) $(objpfx)tst-resolv-threads: \ $(libdl) $(objpfx)libresolv.so $(shared-thread-library) +$(objpfx)tst-resolv-txnid-collision: $(objpfx)libresolv.a \ + $(static-thread-library) $(objpfx)tst-resolv-canonname: \ $(libdl) $(objpfx)libresolv.so $(shared-thread-library) $(objpfx)tst-resolv-trustad: $(objpfx)libresolv.so $(shared-thread-library) diff --git a/resolv/res_send.c b/resolv/res_send.c index 7e5fec6646..70e5066031 100644 --- a/resolv/res_send.c +++ b/resolv/res_send.c @@ -1342,15 +1342,6 @@ send_dg(res_state statp, *terrno = EMSGSIZE; return close_and_return_error (statp, resplen2); } - if ((recvresp1 || hp->id != anhp->id) - && (recvresp2 || hp2->id != anhp->id)) { - /* - * response from old query, ignore it. - * XXX - potential security hazard could - * be detected here. - */ - goto wait; - } /* Paranoia check. Due to the connected UDP socket, the kernel has already filtered invalid addresses @@ -1360,15 +1351,24 @@ send_dg(res_state statp, /* Check for the correct header layout and a matching question. */ - if ((recvresp1 || !res_queriesmatch(buf, buf + buflen, - *thisansp, - *thisansp - + *thisanssizp)) - && (recvresp2 || !res_queriesmatch(buf2, buf2 + buflen2, - *thisansp, - *thisansp - + *thisanssizp))) - goto wait; + int matching_query = 0; /* Default to no matching query. */ + if (!recvresp1 + && anhp->id == hp->id + && res_queriesmatch (buf, buf + buflen, + *thisansp, *thisansp + *thisanssizp)) + matching_query = 1; + if (!recvresp2 + && anhp->id == hp2->id + && res_queriesmatch (buf2, buf2 + buflen2, + *thisansp, *thisansp + *thisanssizp)) + matching_query = 2; + if (matching_query == 0) + /* Spurious UDP packet. Drop it and continue + waiting. */ + { + need_recompute = 1; + goto wait; + } if (anhp->rcode == SERVFAIL || anhp->rcode == NOTIMP || @@ -1383,7 +1383,7 @@ send_dg(res_state statp, /* No data from the first reply. */ resplen = 0; /* We are waiting for a possible second reply. */ - if (hp->id == anhp->id) + if (matching_query == 1) recvresp1 = 1; else recvresp2 = 1; @@ -1414,7 +1414,7 @@ send_dg(res_state statp, return (1); } /* Mark which reply we received. */ - if (recvresp1 == 0 && hp->id == anhp->id) + if (matching_query == 1) recvresp1 = 1; else recvresp2 = 1; diff --git a/resolv/tst-resolv-txnid-collision.c b/resolv/tst-resolv-txnid-collision.c new file mode 100644 index 0000000000..611d37362f --- /dev/null +++ b/resolv/tst-resolv-txnid-collision.c @@ -0,0 +1,329 @@ +/* Test parallel queries with transaction ID collisions. + Copyright (C) 2020 Free Software Foundation, Inc. + This file is part of the GNU C Library. + + The GNU C Library is free software; you can redistribute it and/or + modify it under the terms of the GNU Lesser General Public + License as published by the Free Software Foundation; either + version 2.1 of the License, or (at your option) any later version. + + The GNU C Library is distributed in the hope that it will be useful, + but WITHOUT ANY WARRANTY; without even the implied warranty of + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + Lesser General Public License for more details. + + You should have received a copy of the GNU Lesser General Public + License along with the GNU C Library; if not, see + . */ + +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include + +/* Result of parsing a DNS question name. + + A question name has the form reorder-N-M-rcode-C.example.net, where + N and M are either 0 and 1, corresponding to the reorder member, + and C is a number that will be stored in the rcode field. + + Also see parse_qname below. */ +struct parsed_qname +{ + /* The DNS response code requested from the first server. The + second server always responds with RCODE zero. */ + int rcode; + + /* Indicates whether to perform reordering in the responses from the + respective server. */ + bool reorder[2]; +}; + +/* Fills *PARSED based on QNAME. */ +static void +parse_qname (struct parsed_qname *parsed, const char *qname) +{ + int reorder0; + int reorder1; + int rcode; + char *suffix; + if (sscanf (qname, "reorder-%d-%d.rcode-%d.%ms", + &reorder0, &reorder1, &rcode, &suffix) == 4) + { + if (reorder0 != 0) + TEST_COMPARE (reorder0, 1); + if (reorder1 != 0) + TEST_COMPARE (reorder1, 1); + TEST_VERIFY (rcode >= 0 && rcode <= 15); + TEST_COMPARE_STRING (suffix, "example.net"); + free (suffix); + + parsed->rcode = rcode; + parsed->reorder[0] = reorder0; + parsed->reorder[1] = reorder1; + } + else + FAIL_EXIT1 ("unexpected query: %s", qname); +} + +/* Used to construct a response. The first server responds with an + error, the second server succeeds. */ +static void +build_response (const struct resolv_response_context *ctx, + struct resolv_response_builder *b, + const char *qname, uint16_t qclass, uint16_t qtype) +{ + struct parsed_qname parsed; + parse_qname (&parsed, qname); + + switch (ctx->server_index) + { + case 0: + { + struct resolv_response_flags flags = { 0 }; + if (parsed.rcode == 0) + /* Simulate a delegation in case a NODATA (RCODE zero) + response is requested. */ + flags.clear_ra = true; + else + flags.rcode = parsed.rcode; + + resolv_response_init (b, flags); + resolv_response_add_question (b, qname, qclass, qtype); + } + break; + + case 1: + { + struct resolv_response_flags flags = { 0, }; + resolv_response_init (b, flags); + resolv_response_add_question (b, qname, qclass, qtype); + + resolv_response_section (b, ns_s_an); + resolv_response_open_record (b, qname, qclass, qtype, 0); + if (qtype == T_A) + { + char ipv4[4] = { 192, 0, 2, 1 }; + resolv_response_add_data (b, &ipv4, sizeof (ipv4)); + } + else + { + char ipv6[16] + = { 0x20, 0x01, 0xd, 0xb8, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 1 }; + resolv_response_add_data (b, &ipv6, sizeof (ipv6)); + } + resolv_response_close_record (b); + } + break; + } +} + +/* Used to reorder responses. */ +struct resolv_response_context *previous_query; + +/* Used to keep track of the queries received. */ +static int previous_server_index = -1; +static uint16_t previous_qtype; + +/* For each server, buffer the first query and then send both answers + to the second query, reordered if requested. */ +static void +response (const struct resolv_response_context *ctx, + struct resolv_response_builder *b, + const char *qname, uint16_t qclass, uint16_t qtype) +{ + TEST_VERIFY (qtype == T_A || qtype == T_AAAA); + if (ctx->server_index != 0) + TEST_COMPARE (ctx->server_index, 1); + + struct parsed_qname parsed; + parse_qname (&parsed, qname); + + if (previous_query == NULL) + { + /* No buffered query. Record this query and do not send a + response. */ + TEST_COMPARE (previous_qtype, 0); + previous_query = resolv_response_context_duplicate (ctx); + previous_qtype = qtype; + resolv_response_drop (b); + previous_server_index = ctx->server_index; + + if (test_verbose) + printf ("info: buffering first query for: %s\n", qname); + } + else + { + TEST_VERIFY (previous_query != 0); + TEST_COMPARE (ctx->server_index, previous_server_index); + TEST_VERIFY (previous_qtype != qtype); /* Not a duplicate. */ + + /* If reordering, send a response for this query explicitly, and + then skip the implicit send. */ + if (parsed.reorder[ctx->server_index]) + { + if (test_verbose) + printf ("info: sending reordered second response for: %s\n", + qname); + build_response (ctx, b, qname, qclass, qtype); + resolv_response_send_udp (ctx, b); + resolv_response_drop (b); + } + + /* Build a response for the previous query and send it, thus + reordering the two responses. */ + { + if (test_verbose) + printf ("info: sending first response for: %s\n", qname); + struct resolv_response_builder *btmp + = resolv_response_builder_allocate (previous_query->query_buffer, + previous_query->query_length); + build_response (ctx, btmp, qname, qclass, previous_qtype); + resolv_response_send_udp (ctx, btmp); + resolv_response_builder_free (btmp); + } + + /* If not reordering, send the reply as usual. */ + if (!parsed.reorder[ctx->server_index]) + { + if (test_verbose) + printf ("info: sending non-reordered second response for: %s\n", + qname); + build_response (ctx, b, qname, qclass, qtype); + } + + /* Unbuffer the response and prepare for the next query. */ + resolv_response_context_free (previous_query); + previous_query = NULL; + previous_qtype = 0; + previous_server_index = -1; + } +} + +/* Runs a query for QNAME and checks for the expected reply. See + struct parsed_qname for the expected format for QNAME. */ +static void +test_qname (const char *qname, int rcode) +{ + struct resolv_context *ctx = __resolv_context_get (); + TEST_VERIFY_EXIT (ctx != NULL); + + unsigned char q1[512]; + int q1len = res_mkquery (QUERY, qname, C_IN, T_A, NULL, 0, NULL, + q1, sizeof (q1)); + TEST_VERIFY_EXIT (q1len > 12); + + unsigned char q2[512]; + int q2len = res_mkquery (QUERY, qname, C_IN, T_AAAA, NULL, 0, NULL, + q2, sizeof (q2)); + TEST_VERIFY_EXIT (q2len > 12); + + /* Produce a transaction ID collision. */ + memcpy (q2, q1, 2); + + unsigned char ans1[512]; + unsigned char *ans1p = ans1; + unsigned char *ans2p = NULL; + int nans2p = 0; + int resplen2 = 0; + int ans2p_malloced = 0; + + /* Perform a parallel A/AAAA query. */ + int resplen1 = __res_context_send (ctx, q1, q1len, q2, q2len, + ans1, sizeof (ans1), &ans1p, + &ans2p, &nans2p, + &resplen2, &ans2p_malloced); + + TEST_VERIFY (resplen1 > 12); + TEST_VERIFY (resplen2 > 12); + if (resplen1 <= 12 || resplen2 <= 12) + return; + + if (rcode == 1 || rcode == 3) + { + /* Format Error and Name Error responses does not trigger + switching to the next server. */ + TEST_COMPARE (ans1p[3] & 0x0f, rcode); + TEST_COMPARE (ans2p[3] & 0x0f, rcode); + return; + } + + /* The response should be successful. */ + TEST_COMPARE (ans1p[3] & 0x0f, 0); + TEST_COMPARE (ans2p[3] & 0x0f, 0); + + /* Due to bug 19691, the answer may not be in the slot matching the + query. Assume that the AAAA response is the longer one. */ + unsigned char *a_answer; + int a_answer_length; + unsigned char *aaaa_answer; + int aaaa_answer_length; + if (resplen2 > resplen1) + { + a_answer = ans1p; + a_answer_length = resplen1; + aaaa_answer = ans2p; + aaaa_answer_length = resplen2; + } + else + { + a_answer = ans2p; + a_answer_length = resplen2; + aaaa_answer = ans1p; + aaaa_answer_length = resplen1; + } + + { + char *expected = xasprintf ("name: %s\n" + "address: 192.0.2.1\n", + qname); + check_dns_packet (qname, a_answer, a_answer_length, expected); + free (expected); + } + { + char *expected = xasprintf ("name: %s\n" + "address: 2001:db8::1\n", + qname); + check_dns_packet (qname, aaaa_answer, aaaa_answer_length, expected); + free (expected); + } + + if (ans2p_malloced) + free (ans2p); + + __resolv_context_put (ctx); +} + +static int +do_test (void) +{ + struct resolv_test *aux = resolv_test_start + ((struct resolv_redirect_config) + { + .response_callback = response, + }); + + for (int rcode = 0; rcode <= 5; ++rcode) + for (int do_reorder_0 = 0; do_reorder_0 < 2; ++do_reorder_0) + for (int do_reorder_1 = 0; do_reorder_1 < 2; ++do_reorder_1) + { + char *qname = xasprintf ("reorder-%d-%d.rcode-%d.example.net", + do_reorder_0, do_reorder_1, rcode); + test_qname (qname, rcode); + free (qname); + } + + resolv_test_end (aux); + + return 0; +} + +#include