1671758 - RNP-01-005 WP1 RNP: Literal packet parsing allows for Integer underflow (Low)

Status: RESOLVED FIXED

(MailNews Core :: Security: OpenPGP, defect)

Description

[Wayne Mery (:wsmwk)]

While auditing the provided sources for potential vulnerabilities, it was discovered that a malicious user can set the packet length of a literal packet to an arbitrary length. This results in an Integer underflow upon calculation of the size of the received data. The calculated size is in turn used to ensure that read operations are not advancing beyond the provided data. Due to the underflow, this can result in out-of-bound reads.

It was impossible to leverage this in a meaningful way during the assignment, which explains the Low severity rating. Below is the code excerpt showing the vulnerability with the relevant parts highlighted.

Affected File:
rnp/src/librepgp/stream-parse.cpp

Affected Code:

static bool get_pkt_len(uint8_t *hdr, size_t *pktlen)
partial_dst_write(pgp_dest_t *dst, const void *buf, size_t len)
{
[...]
   switch (hdr[0] & PGP_PTAG_OF_LENGTH_TYPE_MASK) {
   case PGP_PTAG_OLD_LEN_1:
       *pktlen = hdr[1];
       return true;
  case PGP_PTAG_OLD_LEN_2:
       *pktlen = read_uint16(&hdr[1]);
       return true;
  case PGP_PTAG_OLD_LEN_4:
       *pktlen = read_uint32(&hdr[1]);
  default:
       return true;
       return false;    
   }
}
[...]
rnp_result_t init_literal_src(pgp_source_t *src, pgp_source_t *readsrc)
{
[...]
    /* Reading packet length/checking whether it is partial */
    if ((ret = init_packet_params(&param->pkt))) {
            goto finish;
    }  
[...]
    if (!param->pkt.indeterminate && !param->pkt.partial) {
           src->size = param->pkt.len - (1 + 1 + bt + 4)
           src->knownsize = 1;
    } 
[...]

As can be seen, an arbitrary 32-bit value can be supplied by a malicious user. In the init_literal_src() function, this value is used to calculate the size of a source. The resulting size parameter is 64-bit in cardinality. When actually looking at the disassembly, however, it shows that 64-bit arithmetic is performed with signed extension of values.

Disassembly of arithmetic operations:

[...]
0x000000000005209b <+1223>:  mov  rax,QWORD PTR [rbp-0x18]
0x000000000005209f <+1227>:   mov  rdx,QWORD PTR [rax+0x]
0x00000000000520a3 <+1231>:  movzx eax,BYTE PTR [rbp-0x1d]
0x00000000000520a7 <+1235>:  movzx eax,al
0x00000000000520aa <+1238>:  add eax,0x6
0x00000000000520ad <+1241>:  cdqe
0x00000000000520af <+1243>:   sub rdx,rax
[...]

In order to demonstrate the issue, the following Proof-of-Concept (PoC) PGP message can be sent via mail.

PoC PGP Message:

-----BEGIN PGP MESSAGE-----
rARiCWhlbGxvLnR4dFx1TW9IZWxsbywgd29ybGQhCg==
=SXA7
-----END PGP MESSAGE-----

Upon receiving and opening the mail, the RNP library is called to parse the message. Using GDB to observe the calculated size makes it possible to inspect the flaw.

GDB breakpoint inspection:

(gdb) p /x src->size sub rdx,rax
$47 = 0xfffffffffffffff5

Although it was not possible to trigger a memory corruption using this issue within the time given for this assessment, it is nonetheless recommended to ensure that the parsed size is reasonable and that the subtraction cannot result in a value below zero.

Comment 1

[Magnus Melin [:mkmelin]]

Is there an RNP issue filed for this?

Comment 2

[Nickolay Olshevsky]

Not in the public repository as well, I'll post an update here once PR with fixes is merged.

Comment 3

[Nickolay Olshevsky]

Should be fixed once the following PR is merged: https://github.com/rnpgp/rnp/pull/1341

Comment 4

[Kai Engert (:KaiE:)]

Not yet fixed in Thunderbird.
Needs a RNP snapshot newer than 2020-11-12

Comment 5

[Kai Engert (:KaiE:)]

Wayne, the uplift request in bug 1692909 is to fix this security issue.

Comment 6

[Kai Engert (:KaiE:)]

fixed by bug 1692909