Skip to content

Commit dd01bb1

Browse files
inlinedinlined
authored
UpsertBucket uses a purpose label to detect & work around squatting (#9269)
Fixes both a DoS and RCE vulnerability when an attacker could predict the bucket that App Hosting or Functions would use for uploading source. Instead of returning a bucket when it exists (but could be owned by an attacker) we list the buckets actually owned by the user. There are three cases to consider: There exists a bucket with the "purpose label". This will usually have the same name as the target bucket name, but may not in the case that the bucket was created with collision avoidance. Return whichever bucket has the label. There exists a bucket without the "purpose label" but with the target bucket name. Since we got this bucket name from "listBuckets" we know that this is user owned and it is safe to add the purpose label to migrate the fleet to one shape The bucket does not exist. Try to create it. If creation fails (e.g. naming conflict due to squatting) then add a random 6-character string at the end to avoid conflicts. We try this 4 times, so an attacker would need to have squatted on about 600 million buckets for there to be at least a 1% chance that deployment fails (and they can retry without consequence). Manually tested: Create a bucket without conflict Create a bucket with conflict Reuse a bucket Add a purpose label to a legacy bucket
1 parent 3955ccc commit dd01bb1

File tree

7 files changed

+362
-138
lines changed

7 files changed

+362
-138
lines changed

‎src/deploy/apphosting/deploy.spec.ts‎

Lines changed: 21 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -75,25 +75,29 @@ describe("apphosting", () => {
7575

7676
it("upserts regional GCS bucket", async () => {
7777
const context = initializeContext();
78-
getProjectNumberStub.resolves("000000000000");
79-
upsertBucketStub.resolves();
78+
const projectNumber = "000000000000";
79+
const location = "us-central1";
80+
const bucketName = `firebaseapphosting-sources-${projectNumber}-${location}`;
81+
82+
getProjectNumberStub.resolves(projectNumber);
83+
upsertBucketStub.resolves(bucketName);
8084
createArchiveStub.resolves("path/to/foo-1234.zip");
8185
uploadObjectStub.resolves({
82-
bucket: "firebaseapphosting-sources-12345678-us-central1",
86+
bucket: bucketName,
8387
object: "foo-1234",
8488
});
85-
createReadStreamStub.resolves();
89+
createReadStreamStub.returns("stream" as any);
8690

8791
await deploy(context, opts);
8892

8993
expect(upsertBucketStub).to.be.calledWith({
9094
product: "apphosting",
91-
createMessage:
92-
"Creating Cloud Storage bucket in us-central1 to store App Hosting source code uploads at firebaseapphosting-sources-000000000000-us-central1...",
95+
createMessage: `Creating Cloud Storage bucket in ${location} to store App Hosting source code uploads at ${bucketName}...`,
9396
projectId: "my-project",
9497
req: {
95-
name: "firebaseapphosting-sources-000000000000-us-central1",
96-
location: "us-central1",
98+
baseName: bucketName,
99+
purposeLabel: `apphosting-source-${location}`,
100+
location: location,
97101
lifecycle: {
98102
rule: [
99103
{
@@ -108,20 +112,22 @@ describe("apphosting", () => {
108112

109113
it("correctly creates and sets storage URIs", async () => {
110114
const context = initializeContext();
111-
getProjectNumberStub.resolves("000000000000");
112-
upsertBucketStub.resolves();
115+
const projectNumber = "000000000000";
116+
const location = "us-central1";
117+
const bucketName = `firebaseapphosting-sources-${projectNumber}-${location}`;
118+
119+
getProjectNumberStub.resolves(projectNumber);
120+
upsertBucketStub.resolves(bucketName);
113121
createArchiveStub.resolves("path/to/foo-1234.zip");
114122
uploadObjectStub.resolves({
115-
bucket: "firebaseapphosting-sources-12345678-us-central1",
123+
bucket: bucketName,
116124
object: "foo-1234",
117125
});
118-
createReadStreamStub.resolves();
126+
createReadStreamStub.returns("stream" as any);
119127

120128
await deploy(context, opts);
121129

122-
expect(context.backendStorageUris["foo"]).to.equal(
123-
"gs://firebaseapphosting-sources-000000000000-us-central1/foo-1234.zip",
124-
);
130+
expect(context.backendStorageUris["foo"]).to.equal(`gs://${bucketName}/foo-1234.zip`);
125131
});
126132
});
127133
});

‎src/deploy/apphosting/deploy.ts‎

Lines changed: 8 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -24,15 +24,17 @@ export default async function (context: Context, options: Options): Promise<void
2424
}
2525

2626
// Ensure that a bucket exists in each region that a backend is or will be deployed to
27+
const bucketsPerLocation: Record<string, string> = {};
2728
await Promise.all(
2829
Object.values(context.backendLocations).map(async (loc) => {
29-
const bucketName = `firebaseapphosting-sources-${options.projectNumber}-${loc.toLowerCase()}`;
30-
await gcs.upsertBucket({
30+
const baseName = `firebaseapphosting-sources-${options.projectNumber}-${loc.toLowerCase()}`;
31+
const resolvedName = await gcs.upsertBucket({
3132
product: "apphosting",
32-
createMessage: `Creating Cloud Storage bucket in ${loc} to store App Hosting source code uploads at ${bucketName}...`,
33+
createMessage: `Creating Cloud Storage bucket in ${loc} to store App Hosting source code uploads at ${baseName}...`,
3334
projectId,
3435
req: {
35-
name: bucketName,
36+
baseName,
37+
purposeLabel: `apphosting-source-${loc.toLowerCase()}`,
3638
location: loc,
3739
lifecycle: {
3840
rule: [
@@ -48,6 +50,7 @@ export default async function (context: Context, options: Options): Promise<void
4850
},
4951
},
5052
});
53+
bucketsPerLocation[loc] = resolvedName;
5154
}),
5255
);
5356

@@ -65,7 +68,7 @@ export default async function (context: Context, options: Options): Promise<void
6568
"apphosting",
6669
`Uploading source code at ${projectSourcePath} for backend ${cfg.backendId}...`,
6770
);
68-
const bucketName = `firebaseapphosting-sources-${options.projectNumber}-${backendLocation.toLowerCase()}`;
71+
const bucketName = bucketsPerLocation[backendLocation]!;
6972
const { bucket, object } = await gcs.uploadObject(
7073
{
7174
file: zippedSourcePath,

‎src/deploy/functions/deploy.spec.ts‎

Lines changed: 16 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -153,8 +153,6 @@ describe("deploy", () => {
153153
functionsSourceV2: "source.zip",
154154
functionsSourceV2Hash: "source-hash",
155155
};
156-
const CREATE_MESSAGE =
157-
"Creating Cloud Storage bucket in region to store Functions source code uploads at firebase-functions-src-123456...";
158156

159157
before(() => {
160158
experimentEnabled = experiments.isEnabled("runfunctions");
@@ -163,7 +161,7 @@ describe("deploy", () => {
163161

164162
beforeEach(() => {
165163
gcsUploadStub = sinon.stub(gcs, "upload").resolves({ generation: "1" });
166-
gcsUpsertBucketStub = sinon.stub(gcs, "upsertBucket").resolves();
164+
gcsUpsertBucketStub = sinon.stub(gcs, "upsertBucket");
167165
gcfv2GenerateUploadUrlStub = sinon.stub(gcfv2, "generateUploadUrl").resolves({
168166
uploadUrl: "https://storage.googleapis.com/upload/url",
169167
storageSource: {
@@ -179,27 +177,32 @@ describe("deploy", () => {
179177
});
180178

181179
describe("with runfunctions experiment enabled", () => {
180+
const PROJECT_NUMBER = "123456";
181+
const BUCKET_NAME = `firebase-functions-src-${PROJECT_NUMBER}`;
182+
182183
before(() => experiments.setEnabled("runfunctions", true));
183184

184185
it("should call gcs.upsertBucket and gcs.upload for gcfv2 functions", async () => {
185186
const wantBackend = backend.of({ ...ENDPOINT, platform: "gcfv2" });
187+
gcsUpsertBucketStub.resolves(BUCKET_NAME);
186188

187-
await deploy.uploadSourceV2("project", "123456", SOURCE, wantBackend);
189+
await deploy.uploadSourceV2("project", PROJECT_NUMBER, SOURCE, wantBackend);
188190

189191
expect(gcsUpsertBucketStub).to.be.calledOnceWith({
190192
product: "functions",
191193
projectId: "project",
192-
createMessage: CREATE_MESSAGE,
194+
createMessage: `Creating Cloud Storage bucket in region to store Functions source code uploads at ${BUCKET_NAME}...`,
193195
req: {
194-
name: "firebase-functions-src-123456",
196+
baseName: BUCKET_NAME,
195197
location: "region",
198+
purposeLabel: "functions-source-region",
196199
lifecycle: { rule: [{ action: { type: "Delete" }, condition: { age: 1 } }] },
197200
},
198201
});
199202
expect(createReadStreamStub).to.be.calledOnceWith("source.zip");
200203
expect(gcsUploadStub).to.be.calledOnceWith(
201204
{ file: "source.zip", stream: "stream" },
202-
"firebase-functions-src-123456/source-hash.zip",
205+
`${BUCKET_NAME}/source-hash.zip`,
203206
undefined,
204207
true,
205208
);
@@ -208,23 +211,25 @@ describe("deploy", () => {
208211

209212
it("should call gcs.upsertBucket and gcs.upload for run functions", async () => {
210213
const wantBackend = backend.of({ ...ENDPOINT, platform: "run" });
214+
gcsUpsertBucketStub.resolves(BUCKET_NAME);
211215

212-
await deploy.uploadSourceV2("project", "123456", SOURCE, wantBackend);
216+
await deploy.uploadSourceV2("project", PROJECT_NUMBER, SOURCE, wantBackend);
213217

214218
expect(gcsUpsertBucketStub).to.be.calledOnceWith({
215219
product: "functions",
216220
projectId: "project",
217-
createMessage: CREATE_MESSAGE,
221+
createMessage: `Creating Cloud Storage bucket in region to store Functions source code uploads at ${BUCKET_NAME}...`,
218222
req: {
219-
name: "firebase-functions-src-123456",
223+
baseName: BUCKET_NAME,
220224
location: "region",
225+
purposeLabel: "functions-source-region",
221226
lifecycle: { rule: [{ action: { type: "Delete" }, condition: { age: 1 } }] },
222227
},
223228
});
224229
expect(createReadStreamStub).to.be.calledOnceWith("source.zip");
225230
expect(gcsUploadStub).to.be.calledOnceWith(
226231
{ file: "source.zip", stream: "stream" },
227-
"firebase-functions-src-123456/source-hash.zip",
232+
`${BUCKET_NAME}/source-hash.zip`,
228233
undefined,
229234
true,
230235
);

‎src/deploy/functions/deploy.ts‎

Lines changed: 5 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -95,14 +95,15 @@ export async function uploadSourceV2(
9595
// Future behavior: BYO bucket if we're using the Cloud Run API directly because it does not provide a source upload API.
9696
// We use this behavior whenever the "runfunctions" experiment is enabled for now just to help vet the codepath incrementally.
9797
// Using project number to ensure we don't exceed the bucket name length limit (in addition to PII controversy).
98-
const bucketName = `firebase-functions-src-${projectNumber}`;
99-
await gcs.upsertBucket({
98+
const baseName = `firebase-functions-src-${projectNumber}`;
99+
const bucketName = await gcs.upsertBucket({
100100
product: "functions",
101101
projectId,
102-
createMessage: `Creating Cloud Storage bucket in ${region} to store Functions source code uploads at ${bucketName}...`,
102+
createMessage: `Creating Cloud Storage bucket in ${region} to store Functions source code uploads at ${baseName}...`,
103103
req: {
104-
name: bucketName,
104+
baseName,
105105
location: region,
106+
purposeLabel: `functions-source-${region.toLowerCase()}`,
106107
lifecycle: {
107108
rule: [
108109
{

‎src/deploy/functions/params.ts‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -695,7 +695,7 @@ async function promptResourceString(
695695
const notFound = new FirebaseError(`No instances of ${input.resource.type} found.`);
696696
switch (input.resource.type) {
697697
case "storage.googleapis.com/Bucket":
698-
const buckets = await listBuckets(projectId);
698+
const buckets = (await listBuckets(projectId)).map((b) => b.name);
699699
if (buckets.length === 0) {
700700
throw notFound;
701701
}
@@ -723,7 +723,7 @@ async function promptResourceStrings(
723723
const notFound = new FirebaseError(`No instances of ${input.resource.type} found.`);
724724
switch (input.resource.type) {
725725
case "storage.googleapis.com/Bucket":
726-
const buckets = await listBuckets(projectId);
726+
const buckets = (await listBuckets(projectId)).map((b) => b.name);
727727
if (buckets.length === 0) {
728728
throw notFound;
729729
}

0 commit comments

Comments
 (0)