Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Vulnerability to attacks because there is no size limit #48788

Closed
BijanVan opened this issue Mar 6, 2018 · 8 comments
Closed

Vulnerability to attacks because there is no size limit #48788

BijanVan opened this issue Mar 6, 2018 · 8 comments
Labels
A-docs Area: documentation for any part of the project, including the compiler, standard library, and tools C-enhancement Category: An issue proposing an enhancement or a PR with one. E-medium Call for participation: Medium difficulty. Experience needed to fix: Intermediate. P-medium Medium priority T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.

Comments

@BijanVan
Copy link

BijanVan commented Mar 6, 2018

pub fn read_line(&self, buf: &mut String) -> io::Result<usize> {

read_line function seems vulnerable to attack. If an attacker sends data continually without "\r\n" the size of buffer (String) would keep growing without a limit.
tokio

@nagisa
Copy link
Member

nagisa commented Mar 6, 2018

This is a function on the standard input though? What is the threat model? That you DoS your own computer?

@BijanVan
Copy link
Author

BijanVan commented Mar 6, 2018

That is not limited to Tokio. TcpStream from standard library could be wrapped by BufReader.
Here is an example:

`use std::io::{BufRead, BufReader};
use std::net::TcpListener;

fn main() {
let listener = TcpListener::bind("localhost:8080").unwrap();

for stream in listener.incoming() {
    let mut stream = stream.unwrap();
    let mut string_buf = String::new();
    let mut reader = BufReader::new(stream);
    let line = reader.read_line(&mut string_buf);
}

}
`

@Centril Centril added A-security Area: Security (example: address space layout randomization). T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. labels Mar 6, 2018
@pietroalbini pietroalbini added I-nominated C-bug Category: This is a bug. labels Mar 6, 2018
@Centril Centril added C-bug Category: This is a bug. and removed C-bug Category: This is a bug. labels Mar 6, 2018
@Mark-Simulacrum
Copy link
Member

This somewhat feels like something the user should be aware of. It's also not clear to me that there's much we can/should do about this -- an arbitrary limit on our side would be decidedly odd (and potentially break some use cases). Users using and of the read methods should be aware that they are both blocking and may not return (ever, potentially). This seems semi-obvious to me... but perhaps we could clarify it in documentation.

@BijanVan
Copy link
Author

BijanVan commented Mar 7, 2018

I guess adding
fn read_line(&mut self, buf: &mut String, max_length: usize) -> Result
in addition to:
fn read_line(&mut self, buf: &mut String) -> Result
would not break anything.

@Mark-Simulacrum
Copy link
Member

That should be done via Read::take, so I'm not sure that it's entirely useful. Anyway, it almost seems like this is a too specific thing to add - in most cases, I'd imagine that if you want to limit the potential line length, you'd want a different return type, among maybe other things. Since I'd imagine they'd be fairly specific to each use case it seems easier for users to implement that on their own via read or a similar call.

@alexcrichton
Copy link
Member

We discussed this during libs triage today and the conclusion was that we didn't consider this a bug to fix in libstd but would of course be more than willing to update the documentation. As a result I'm reclassifying as a documentation issue.

@alexcrichton alexcrichton added A-docs Area: documentation for any part of the project, including the compiler, standard library, and tools and removed C-bug Category: This is a bug. I-nominated T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. A-security Area: Security (example: address space layout randomization). labels Mar 21, 2018
@frewsxcv
Copy link
Member

Note to future selves: if we put a note about this on BufRead::read_line, we should consider adding a note on BufRead::read_until too since it's susceptible to the same issue

@XAMPPRocky XAMPPRocky added the C-enhancement Category: An issue proposing an enhancement or a PR with one. label May 21, 2018
@steveklabnik steveklabnik added the E-medium Call for participation: Medium difficulty. Experience needed to fix: Intermediate. label May 28, 2018
@steveklabnik steveklabnik added the P-medium Medium priority label Dec 27, 2018
@jonas-schievink jonas-schievink added the T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. label Mar 6, 2020
Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this issue Jun 2, 2020
…_until-doc, r=Dylan-DPC

Add a warning about infinite reading in read_(until|line)

Fix for rust-lang#48788.

This PR adds a warning to [`BufRead::read_line`](https://doc.rust-lang.org/stable/std/io/trait.BufRead.html#method.read_line) and [`BufRead::read_until`](https://doc.rust-lang.org/stable/std/io/trait.BufRead.html#method.read_until) about the possibility of an attack using the behaviour of these functions.

I did not mention a possible fix (using [`Read::take`](https://doc.rust-lang.org/stable/std/io/trait.Read.html#method.take), should I ?
@poliorcetics
Copy link
Contributor

poliorcetics commented Jun 3, 2020

If the added doc is deemed sufficient, this should be closed, though I do not know who to ping for that.

@sfackler sfackler closed this as completed Jun 3, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-docs Area: documentation for any part of the project, including the compiler, standard library, and tools C-enhancement Category: An issue proposing an enhancement or a PR with one. E-medium Call for participation: Medium difficulty. Experience needed to fix: Intermediate. P-medium Medium priority T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests