servo / font-kit

A cross-platform font loading library written in Rust
Apache License 2.0
681 stars 100 forks source link

core_text::Font::from_file is unsound #96

Closed nox closed 5 years ago

nox commented 5 years ago

It creates a memory-mapped buffer from a file, and this file can be modified by any other process on the computer or even another thread in the same executable, but it isn't marked unsafe.

This is exactly why Mmap::map itself is unsafe.

pcwalton commented 5 years ago

I'll fix this.

Shnatsel commented 5 years ago

I believe resvg does something very similar: https://github.com/RazrFalcon/resvg/blob/be61ee86ee4a99cbea7ca75f5dc06c1ef8bb0bc0/usvg/src/fontdb.rs#L119

@nox could you take a look?

Shnatsel commented 5 years ago

@pcwalton since this resolves a potential memory safety issue, perhaps it would make sense to file a security advisory? https://github.com/RustSec/advisory-db

RazrFalcon commented 5 years ago

@Shnatsel resvg uses memmap for a very short period of time. It doesn't store actually store the file data.

pcwalton commented 5 years ago

I have a hard time imagining a realistic situation that would cause this to be an actual security problem. In general I think filing security advisories for things that are not actually vulnerabilities reduces security by increasing noise.

The only security issue I could think of would involve local privilege escalation if font-kit is used in some sort of privileged daemon that loads fonts under control of users. I don't know of anyone using font-kit like this. I would be happy to reconsider if someone can present an actual security issue, but I'm not inclined to file a security advisory without that.

On Sat, Aug 31, 2019, 12:00 PM Shnatsel notifications@github.com wrote:

@pcwalton https://github.com/pcwalton since this resolves a potential memory safety issue, perhaps it would make sense to file a security advisory? https://github.com/RustSec/advisory-db

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/pcwalton/font-kit/issues/96?email_source=notifications&email_token=AABGRSPAR5B3M57GVQNSAX3QHK5WDA5CNFSM4ISN5CIKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD5TTH2Q#issuecomment-526857194, or mute the thread https://github.com/notifications/unsubscribe-auth/AABGRSOR24YNNR2QDIGPKT3QHK5WDANCNFSM4ISN5CIA .

pcwalton commented 5 years ago

Besides, font-kit doesn't run fonts through libots, so nobody should be loading untrusted fonts through font-kit to begin with.

On Sat, Aug 31, 2019, 9:26 PM Patrick Walton pcwalton@mimiga.net wrote:

I have a hard time imagining a realistic situation that would cause this to be an actual security problem. In general I think filing security advisories for things that are not actually vulnerabilities reduces security by increasing noise.

The only security issue I could think of would involve local privilege escalation if font-kit is used in some sort of privileged daemon that loads fonts under control of users. I don't know of anyone using font-kit like this. I would be happy to reconsider if someone can present an actual security issue, but I'm not inclined to file a security advisory without that.

On Sat, Aug 31, 2019, 12:00 PM Shnatsel notifications@github.com wrote:

@pcwalton https://github.com/pcwalton since this resolves a potential memory safety issue, perhaps it would make sense to file a security advisory? https://github.com/RustSec/advisory-db

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/pcwalton/font-kit/issues/96?email_source=notifications&email_token=AABGRSPAR5B3M57GVQNSAX3QHK5WDA5CNFSM4ISN5CIKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD5TTH2Q#issuecomment-526857194, or mute the thread https://github.com/notifications/unsubscribe-auth/AABGRSOR24YNNR2QDIGPKT3QHK5WDANCNFSM4ISN5CIA .

On Sat, Aug 31, 2019, 9:26 PM Patrick Walton pcwalton@mimiga.net wrote:

I have a hard time imagining a realistic situation that would cause this to be an actual security problem. In general I think filing security advisories for things that are not actually vulnerabilities reduces security by increasing noise.

The only security issue I could think of would involve local privilege escalation if font-kit is used in some sort of privileged daemon that loads fonts under control of users. I don't know of anyone using font-kit like this. I would be happy to reconsider if someone can present an actual security issue, but I'm not inclined to file a security advisory without that.

On Sat, Aug 31, 2019, 12:00 PM Shnatsel notifications@github.com wrote:

@pcwalton https://github.com/pcwalton since this resolves a potential memory safety issue, perhaps it would make sense to file a security advisory? https://github.com/RustSec/advisory-db

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/pcwalton/font-kit/issues/96?email_source=notifications&email_token=AABGRSPAR5B3M57GVQNSAX3QHK5WDA5CNFSM4ISN5CIKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD5TTH2Q#issuecomment-526857194, or mute the thread https://github.com/notifications/unsubscribe-auth/AABGRSOR24YNNR2QDIGPKT3QHK5WDANCNFSM4ISN5CIA .