Preface

Definitely Typed, a project which manages npm packages inside the @types scope, is supported by Microsoft. However, it is not in the scope of the safe harbor for Microsoft’s bug bounty program.¹

This article describes the vulnerabilities that were reported as potential vulnerabilities, using publicly available information. This was done without actually exploiting / demonstrating the vulnerabilities.

This article is not intended to encourage you to perform an unauthorized vulnerability assessment.
If you find any vulnerabilities in Definitely Typed related products, please report them to members of Definitely Typed.

TL;DR

There were vulnerabilities in the pull request management bot of Definitely Typed, which allowed an attacker to merge a malicious pull request into DefinitelyTyped/DefinitelyTyped.
This would result in tampering with arbitrary packages under the @types scope of npm.

About Definitely Typed

Definitely Typed is a group that maintains a large number of TypeScript type definition packages to complement the type npm packages that do not support TypeScript. It is managed by many people, including the TypeScript maintainers.

Packages under the @types scope are downloaded about 3.7 billion times per month in total² and are used in a large number of projects.

For example, VSCode, Angular and TypeScript itself is depending on Definitely Typed.

Reasons for investigation

While I was discussing with @lambdasawa about systems that would have a significant impact if compromised, we came up with Definitely Typed.
After searching about Definitely Typed a bit, I found that it was used in a very large amount of projects, so I decided to investigate whether it could be compromised.

Initial investigation

As @lambdasawa told me about a GitHub repository that is used to manage Definitely Typed files, I checked DefinitelyTyped/DefinitelyTyped.
Then, I noticed that normal GitHub users, who don’t have any permissions against DefinitelyTyped repository, are using @typescript-bot commands to merge pull requests.
Having had the experience of reporting the Homebrew vulnerability, I thought that this bot might have a similar vulnerability. So I decided to read the source code (DefinitelyTyped/dt-mergebot).

Project overview

Before starting the investigation of dt-mergebot, I found the image below in the project overview.

It looks like Definitely Typed has the concept of a package maintainer, separate from the repository maintainer.
Package maintainers are users who have permission to manage only a single type definition package such as @types/node, and are specified in each package.
If a user has package maintainer permissions for the target package, they can approve pull requests that contain only changes to the target package without using the Definitely Typed maintainer permissions.³

As described in the article of Homebrew vulnerability, it is difficult to properly parse pull requests, and it may induce mistakes.
Since it’s easy to obtain a package maintainer permission by adding a type definition, I decided to see if it’s possible to bypass package-based permissions management by using a specially crafted pull request.

Investigation of dt-mergebot

After reading the source code of dt-mergebot, I found that it behaves as follows.

1. Fetch changed files for a pull request
2. Check which type definition package has the changed files
3. Send a review request to the package maintainer specified in the type definition package
4. If the package maintainer approved the change, the author of the pull request is given permission to merge
5. When the pull request author sends the comment Ready to merge, typescript-bot merges the pull request

While it looks safe as it fetches changed files from the API, I found the code shown below.

const { nodes, totalCount } = prInfo.files!;
if (nodes!.length < totalCount) console.warn(`  *** Note: ${totalCount - nodes!.length} files were not seen by this query!`);

It looks like it checks the totalCount parameter returned by the API and compares it with the total number of files that have actually been returned.
And if the number of returned files is less than totalCount, it executes console.warn.

After checking how dt-mergebot fetches the files, I found the following GraphQL query.

query PR($pr_number: Int!) {
    repository(owner: "DefinitelyTyped", name: "DefinitelyTyped") {
      id
      pullRequest(number: $pr_number) {
        [...]
        files(first: 100) {
          totalCount
          nodes {
            path
            additions
            deletions
          }
        }
        [...]
      }
   }
}

You can see from this query, that dt-mergebot was only fetching the first 100 files when calculating the permissions required to merge.
As mentioned above, if the number of files obtained is less than totalCount, it executes console.warn and displays a warning on the console.

However, console.warn does not stop subsequent processing. So if someone was to make a pull request that changes 101 or more files, the 101st and subsequent files would not be subject to permission checks. Hence the permissions check would be broken. As a result, it allows the bypassing of package-based permissions management.⁴

Report & fix

As mentioned at the beginning of this article, Definitely Typed doesn’t have a clear policy for vulnerability assessments.
So, I decided to not demonstrate it, and report it as a potential vulnerability.

I used the following assumptions as an attack procedure.

1. Find a library on npm that doesn’t have a type definition⁵
2. Create a type definition for the library
3. Send a pull request to DefinitelyTyped repository to add its type definition and wait for the pull request to be merged⁶
4. Once the pull request has merged, create a new pull request that adds 100 files to the directory containing the type definition that you added in step 3
5. Make changes to the file you want to tamper with on the pull request
6. After typescript-bot has checked the pull request, permission to approve it will be granted to you, so you may approve the pull request
7. Add Ready to merge as a comment
8. The pull request will be merged, and the targeted file will have been modified

I’ve sent an email that summarizes these steps to Definitely Typed members.
After that, this vulnerability was fixed in this commit.

Vulnerability… again

After reporting/fixing a vulnerability described above, I asked the members of Definitely Typed if I could publish an article about the vulnerability.
As they allowed me to publish the article, I was reading the code to write the article.
While reading the code, I found that there was a regression.

From this commit, dt-mergebot started fetching all modified files for a pull request by paginating the GitHub API.
You would expect for this to cause no issues because all the files are fetched. But in fact, there is a pitfall in the GitHub API.

As documented, when retrieving a list of modified files with a pull request, GitHub API returns only a maximum of 3000 files.
This means that even if all files are correctly retrieved by using pagination, the client will only receive the first 3000 files.

So, if someone sent a pull request like the image above, GraphQL will return the response like this:

In addition, the totalCount parameter returned by the GraphQL API has an upper limit of 3000 too.
Therefore, by sending a pull request that modified 3001 or more files to DefinitelyTyped/DefinitelyTyped, permission checks could be bypassed and arbitrary modifications could be made to DefinitelyTyped repository.

This has been fixed in this commit, and dt-mergebot now marks pull requests that contain 500 or more modified files as suspicious.

Additional vulnerability

After writing the article above, I started checking other parts of Definitely Typed to see if there are any more vulnerabilities… And found another vulnerability.

There is a tool called types-publisher, which handles publishing of packages after dt-mergebot processes a pull request.

In this tool, the files in the target package are copied into the output directory, but the symbolic link was not taken into consideration during the copy process, so it was possible to publish the unintended files from the system.

However, since the following processing was performed before reaching the copy process, the files that could be published were limited to the files that could be parsed as TypeScript.

const src = createSourceFile(resolvedFilename, readFileAndThrowOnBOM(resolvedFilename, fs));
if (resolvedFilename.endsWith(".d.ts")) {
  types.set(resolvedFilename, src);
} else {
  tests.set(resolvedFilename, src);
}

I also reported this vulnerability via email, and the team fixed it by disallowing symbolic links in this commit.

Conclusion

The vulnerabilities described in this article had a significant impact on the TypeScript ecosystem.
There are many vulnerabilities in the supply chain that have a huge impact, such as this one.
This is why it’s so important that as many people as possible audit the supply chain.
If you have any questions/comments about this article, please send a message to @ryotkak on Twitter.

Timeline