From patchwork Mon Dec 9 22:25:58 2019 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Andreas Rheinhardt X-Patchwork-Id: 16688 Return-Path: X-Original-To: patchwork@ffaux-bg.ffmpeg.org Delivered-To: patchwork@ffaux-bg.ffmpeg.org Received: from ffbox0-bg.mplayerhq.hu (ffbox0-bg.ffmpeg.org [79.124.17.100]) by ffaux.localdomain (Postfix) with ESMTP id 9824344693C for ; Tue, 10 Dec 2019 00:54:14 +0200 (EET) Received: from [127.0.1.1] (localhost [127.0.0.1]) by ffbox0-bg.mplayerhq.hu (Postfix) with ESMTP id 6E1CC68B709; Tue, 10 Dec 2019 00:54:14 +0200 (EET) X-Original-To: ffmpeg-devel@ffmpeg.org Delivered-To: ffmpeg-devel@ffmpeg.org Received: from mail-lj1-f176.google.com (mail-lj1-f176.google.com [209.85.208.176]) by ffbox0-bg.mplayerhq.hu (Postfix) with ESMTPS id 6F33568ADC8 for ; Tue, 10 Dec 2019 00:54:07 +0200 (EET) Received: by mail-lj1-f176.google.com with SMTP id u17so17597799lja.4 for ; Mon, 09 Dec 2019 14:54:07 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=from:to:cc:subject:date:message-id:mime-version :content-transfer-encoding; bh=XXmAChPDX8FPAc7gvhH7SU5ZNkrJ+cND9wQ+wV7GwyE=; b=Ur0+LfXnvGjesKZbP43G0J/i2I6iIilw0Ywka+plKMbGPkCG+sMylGFKDAX7vNPv2Z f3bv8RRVLP2l3iRXgdG7Wk+poPeTVuMjHrCmjf3CXx/EDQk3oWTsJ99puEt3cAOmezhi Kaxc9I5NtBNALrUIU5qvnlY9MkDeX2nXUaazas/vJXxspW8TypmWSnJNqEvnHDlIAvk3 0ckiew49L53+60UMdvXHPsHFLVSuE4mlQftkXxew+KKMkVR1OsHYH6EAhIblJyUm/Ncc 9rVI77pqsjNfsY+WZlf0WFXJT4vp3YdsWlZxOQcnqq38NfkiudNXxcn7z43RErpygAPa 55wQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:from:to:cc:subject:date:message-id:mime-version :content-transfer-encoding; bh=XXmAChPDX8FPAc7gvhH7SU5ZNkrJ+cND9wQ+wV7GwyE=; b=Qo+vL4TRgmjRHephsGnLy50oDUdKQ94yKVi6nHzBvMdOLGpdrktBLJAE/F0WKZUo+N uQy10l9tXdiyxjNSuXnbBDUAi9HwwpoKiXxBRRSc4DyxPnACMpA2nxT91Vffn+OAx9bo LeQxZqia0GBKFNHbIHbJLJtV8r8zembfI84nG1jRJ2vCfXSl/HGXkFaLTg0ZBA8cUpr8 rzeLNDobRnk51XwqLGwikCN2uXfpAgd4JLlP28KMwsMKZmW+wWL58cbjnL/cghlLyORZ j7r6FnaAhErHJzPQUGHdRVPO03YdvezadGgxriSbBzDLET26yWmXRBqT9gDXG0asvVRL BIpg== X-Gm-Message-State: APjAAAVMzLc2AZhs4CtqDMtT4ONDs1QAKFxQYEvEmxOEOW4wfhuvGMF6 Akfmc+5Xxi/QyqvsfXVci3X5oJPu X-Google-Smtp-Source: APXvYqzCPSZYXu+J1Pa7KHpLAHdgHRfn7uxIjDuWfuhH/3CdQpP4VH8iLWuTkFh1iSY8Eriti0ZETg== X-Received: by 2002:adf:eb51:: with SMTP id u17mr4516323wrn.29.1575930379998; Mon, 09 Dec 2019 14:26:19 -0800 (PST) Received: from sblaptop.fritz.box (ipbcc08e23.dynamic.kabel-deutschland.de. [188.192.142.35]) by smtp.gmail.com with ESMTPSA id v22sm865893wml.11.2019.12.09.14.26.18 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Mon, 09 Dec 2019 14:26:19 -0800 (PST) From: Andreas Rheinhardt To: ffmpeg-devel@ffmpeg.org Date: Mon, 9 Dec 2019 23:25:58 +0100 Message-Id: <20191209222604.28920-1-andreas.rheinhardt@gmail.com> X-Mailer: git-send-email 2.20.1 MIME-Version: 1.0 Subject: [FFmpeg-devel] [PATCH 1/7] avcodec/cbs: Check for overflow when reading X-BeenThere: ffmpeg-devel@ffmpeg.org X-Mailman-Version: 2.1.20 Precedence: list List-Id: FFmpeg development discussions and patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Reply-To: FFmpeg development discussions and patches Cc: Andreas Rheinhardt Errors-To: ffmpeg-devel-bounces@ffmpeg.org Sender: "ffmpeg-devel" While CBS itself uses size_t for sizes, it relies on other APIs that use int for their sizes; in particular, AVBuffer uses int for their size parameters and so does GetBitContext with their number of bits. While CBS aims to be a safe API, the checks it employed were not sufficient to prevent overflows: E.g. if the size of a unit was > UINT_MAX / 8, 8 * said size may be truncated to a positive integer before being passed to init_get_bits() in which case its return value would not indicate an error. These checks have been improved to really capture these kinds of errors; furthermore, although the sizes are still size_t, they are now de-facto restricted to 0..INT_MAX - AV_INPUT_BUFFER_PADDING_SIZE. Signed-off-by: Andreas Rheinhardt --- The check in cbs_insert_unit() can currently not be triggered, because av_malloc_array makes sure that it doesn't allocate more than INT_MAX; so the allocation will fail long before we get even close to INT_MAX. av1 and H.26x have not been changed, because their split functions already check the size (in case of H.264 and H.265 this happens in ff_h2645_packet_split()). It would btw be possible to open the GetBitContext generically. The read_unit function would then get the already opened GetBitContext as a parameter. libavcodec/cbs.c | 6 ++++++ libavcodec/cbs_jpeg.c | 2 +- libavcodec/cbs_mpeg2.c | 2 +- libavcodec/cbs_vp9.c | 2 +- 4 files changed, 9 insertions(+), 3 deletions(-) diff --git a/libavcodec/cbs.c b/libavcodec/cbs.c index 0badb192d9..805049404b 100644 --- a/libavcodec/cbs.c +++ b/libavcodec/cbs.c @@ -206,6 +206,9 @@ static int cbs_fill_fragment_data(CodedBitstreamContext *ctx, { av_assert0(!frag->data && !frag->data_ref); + if (size > INT_MAX - AV_INPUT_BUFFER_PADDING_SIZE) + return AVERROR(ERANGE); + frag->data_ref = av_buffer_alloc(size + AV_INPUT_BUFFER_PADDING_SIZE); if (!frag->data_ref) @@ -693,6 +696,9 @@ static int cbs_insert_unit(CodedBitstreamContext *ctx, memmove(units + position + 1, units + position, (frag->nb_units - position) * sizeof(*units)); } else { + if (frag->nb_units == INT_MAX) + return AVERROR(ERANGE); + units = av_malloc_array(frag->nb_units + 1, sizeof(*units)); if (!units) return AVERROR(ENOMEM); diff --git a/libavcodec/cbs_jpeg.c b/libavcodec/cbs_jpeg.c index b189cbd9b7..2bb6c8d18c 100644 --- a/libavcodec/cbs_jpeg.c +++ b/libavcodec/cbs_jpeg.c @@ -246,7 +246,7 @@ static int cbs_jpeg_read_unit(CodedBitstreamContext *ctx, GetBitContext gbc; int err; - err = init_get_bits(&gbc, unit->data, 8 * unit->data_size); + err = init_get_bits8(&gbc, unit->data, unit->data_size); if (err < 0) return err; diff --git a/libavcodec/cbs_mpeg2.c b/libavcodec/cbs_mpeg2.c index 13d871cc89..255f033734 100644 --- a/libavcodec/cbs_mpeg2.c +++ b/libavcodec/cbs_mpeg2.c @@ -227,7 +227,7 @@ static int cbs_mpeg2_read_unit(CodedBitstreamContext *ctx, GetBitContext gbc; int err; - err = init_get_bits(&gbc, unit->data, 8 * unit->data_size); + err = init_get_bits8(&gbc, unit->data, unit->data_size); if (err < 0) return err; diff --git a/libavcodec/cbs_vp9.c b/libavcodec/cbs_vp9.c index 42e4dcf5ac..f6cfaa3b36 100644 --- a/libavcodec/cbs_vp9.c +++ b/libavcodec/cbs_vp9.c @@ -488,7 +488,7 @@ static int cbs_vp9_read_unit(CodedBitstreamContext *ctx, GetBitContext gbc; int err, pos; - err = init_get_bits(&gbc, unit->data, 8 * unit->data_size); + err = init_get_bits8(&gbc, unit->data, unit->data_size); if (err < 0) return err;