The new ContextAware API will be available in Activity 1.2.0-alpha08 and is used by Fragment 1.3.0-alpha08 and AppCompat 1.3.0-alpha02 to implement FragmentManager's and AppCompatDelegate's pre-onCreate() logic.

Wait, this can't work as advertised.

FragmentActivity extends ComponentActivity

public FragmentActivity(@LayoutRes int contentLayoutId) {
    super(contentLayoutId);
    init();
}

private void init() {
    addOnContextAvailableListener(new OnContextAvailableListener() {
        // ...
    }
}

ComponentActivity

@Override
protected void onCreate(@Nullable Bundle savedInstanceState) {
    // ...
    mContextAwareHelper.dispatchOnContextAvailable(this);
    super.onCreate(savedInstanceState);
    //...
}

1. Call FragmentActivity constructor,
2. which calls ComponentActivity super constructor,
3. which dispatches context available... to noone,
4. and calls platform Activity.onCreate.
5. Bubble up to FragmentActivity constructor and register context listener,
6. which is invoked synchronously but only after platform Activity.onCreate has already been called.

Did I miss something? I'm confused.

Never mind, constructor vs onCreate. :facepalm: Sorry.

Re #8 - the fact that these are different things is indeed exactly why this API exists :)

https://docs.google.com/document/d/1hyftTRyqV7ZJ5s2b_-Q9K6RJdpgedgvKP4vYoTK7i1A/edit?usp=drive_web ([Feedback] Bass Booster)

https://docs.google.com/document/d/1hyftTRyqV7ZJ5s2b_-Q9K6RJdpgedgvKP4vYoTK7i1A/edit?usp=drive_web ([Feedback] Bass Booster)

Thanks for verifying the reproduce!

not sure how long you'd have to wait for that

TBH I didn't try waiting for the unpatched version to finish, my expectation is this will be extremely slow as I don't have a dedicated graphic card on my PC.

My assumption is that this can be optimized by using different combinations of paths and iterations, i.e., reducing the gather_lines_and_quads return value and increasing the number of paths.

Now the patch in #5 should demonstrate the bug's existence (as the patch does not change the quadCount value), so it can be fixed without waiting for a stable poc (I'm trying to reduce the time, but I guess it will take times).

In this case I don't think the actual GPU is relevant as the code in question is accumulating draw data on the CPU. But running the SKP for a while and noting the progress, it seems it would take on the order of days or possibly weeks of continuous running for the overflow to trigger on my computer, as it stands.

the code in question is accumulating draw data on the CPU

ok thanks for the clarification, in this case I agree that this would take days or more. what I observed is the same and my PC is i7-13700+64GB so neither CPU nor Memory should be bottleneck

Another thing I notice:

     int conicCount = conics.size() / 3; // updated in gather_lines_and_quads
     int quadAndConicCount = conicCount + quadCount;

this is also a potential overflow place that needs a patch. It may reduce the required time to exploit (not magnitude level, but something like from 3day to 2day, as attacker only need to achieve INT_MAX * 2/3 in the for loop, and the rest 1/3 INT_MAX can be add there) it's a potential security flaw, therefore it's good to add a verification there.

I tried reproducing this on a regular release build, which is much faster than an ASAN build. I realized that the size of quads (and conics) is bound by INT_MAX here: https://crsrc.org/c/third_party/skia/include/private/base/SkTArray.h;l=710;drc=f5e280b6

Since in the quads case this size is always 3 * quadCount, this release check will always fire before quadCount is able to overflow. It is however possible to trigger overflow by drawing very-nearly-quadratic curves which get subdivided here: https://crsrc.org/c/third_party/skia/src/gpu/ganesh/ops/AAHairLinePathRenderer.cpp;l=360;drc=f5e280b6

This takes a delicate balance of increasing quadCount enough to overflow without increasing quads's size enough to overflow. The following seems to work:

    SkPaint paint;
    paint.setAntiAlias(true);
    paint.setStyle(SkPaint::kStroke_Style);
    paint.setStrokeWidth(0);
    
    SkPath path;
    path.moveTo(0, 0);
    for (float y = 0.0f; y < 200.0f; y += 0.01f) {
        const float y1 = y + 200.0f;
        constexpr float x0 = 0.0f;
        constexpr float x1 = 400.0f / 3.0f; 
        constexpr float x2 = 800.0f / 3.0f;
        constexpr float x3 = 400.0f;
        path.cubicTo(x1, y1, x2, y1, x3, y);
        path.cubicTo(x2, y1 + 0.005f, x1, y1 + 0.005f, x0, y + 0.005f);
    }   
    path.close();    
                                            
    for (int i = 0; i < 15000; ++i) {
        canvas->drawPath(path, paint);
    }

However, I'm not sure if this by itself is enough to trigger an OOB read, because the count is cast to size_t before being used for an allocation here: https://crsrc.org/c/third_party/skia/src/gpu/ganesh/GrBufferAllocPool.cpp;l=445;drc=f5e280b6

However, I'm not sure if this by itself is enough to trigger an OOB read, because the count is cast to size_t before being used for an allocation here: https://crsrc.org/c/third_party/skia/src/gpu/ganesh/GrBufferAllocPool.cpp;l=445;drc=f5e280b6

I don't get your point there,

    for (...) {

        quadCount += gather_lines_and_quads(args.fPath, args.fViewMatrix, args.fDevClipBounds,
                                            args.fCapLength, convertConicsToQuads, &lines, &quads,
                                            &conics, &qSubdivs, &cWeights);  //[1]
    }
// -----------------------------------------------------------------------------
    int quadAndConicCount = conicCount + quadCount; [2]
    if (lineCount > kMaxLines || quadAndConicCount > kMaxQuadsAndConics) {
        return;
    }
// -----------------------------------------------------------------------------

        int vertexCount = kQuadNumVertices * quadAndConicCount;
// -----------------------------------------------------------------------------

        void* vertices = target->makeVertexSpace(sizeof(BezierVertex), vertexCount, // [3]
                                                 &vertexBuffer, &firstVertex); 
->GrOpFlushState::makeVertexSpace
->GrVertexBufferAllocPool::makeSpace(

My understanding is that overflow happens either at [1] or [2], so at [3], the vertexCount is already down-casted (overflowed) to an valid int. Therefore, the argument passed to GrVertexBufferAllocPool::makeSpace( being casted to size_t will not lead to any differences.

For instance, at [2] the quadCount is MAX_INT and conicCount is 0x10, then the quadAndConicCount will be downcast to 0xF (overflow),
end up leading the vertexCount at [3] being 0xF * kQuadNumVertices, no matter if this value is cast to size_t, the buffer allocated in [3] will be too small to hold the temporary calculation results.

I mostly agree with your analysis, however an important point is that at [2], if quadCount is INT_MAX, the calculation will actually be 0x7FFFFFFF + 0x10. Then we end up with quadAndConicCount equal to 0x8000000F, or -2147483633. So we actually have to increase quadCount well beyond INT_MAX. We can do that by drawing even larger curves which are subdivided more than those in my first example. The following code seems to result in an OOB read by wrapping quadCount back to a relatively small positive value:

    SkPath path;
    path.moveTo(0, 0);
    for (float y = 0.0f; y < 800.0f; y += 0.05f) {
        const float y1 = y + 800.0f;
        constexpr float x0 = 0.0f;
        constexpr float x1 = 1600.0f / 3.0f;
        constexpr float x2 = 3200.0f / 3.0f;
        constexpr float x3 = 1600.0f;
        path.cubicTo(x1, y1, x2, y1, x3, y);
        path.cubicTo(x2, y1 + 0.025f, x1, y1 + 0.025f, x0, y + 0.025f);
    }
    path.close();

    for (int i = 0; i < 8500; ++i) {
        canvas->drawPath(path, paint);
    }

Note that since an ASAN build was too slow to reproduce this, I used the following patch to verify an OOB write:

--- a/src/gpu/ganesh/ops/AAHairLinePathRenderer.cpp
+++ b/src/gpu/ganesh/ops/AAHairLinePathRenderer.cpp
@@ -701,6 +701,8 @@ void add_conics(const SkPoint p[3],
     }
 }
 
+const BezierVertex* dbg_end = nullptr;
+
 void add_quads(const SkPoint p[3],
                int subdiv,
                const SkMatrix* toDevice,
@@ -727,6 +729,7 @@ void add_quads(const SkPoint p[3],
 
         if (bloat_quad(choppedQuadPts, toDevice, toSrc, outVerts)) {
             set_uv_quad(choppedQuadPts, outVerts);
+            SkASSERT_RELEASE(*vert <= dbg_end - kQuadNumVertices);
             memcpy(*vert, outVerts, kQuadNumVertices * sizeof(BezierVertex));
             *vert += kQuadNumVertices;
         }
@@ -1289,6 +1292,7 @@ void AAHairlineOp::onPrepareDraws(GrMeshDrawTarget* target) {
 
         // Setup vertices
         BezierVertex* bezVerts = reinterpret_cast<BezierVertex*>(vertices);
+        dbg_end = bezVerts + vertexCount;
 
         int unsubdivQuadCnt = quads.size() / 3;
         for (int i = 0; i < unsubdivQuadCnt; ++i) {

Project: skia
Branch: main
Author: James Godfrey-Kittle <jamesgk@google.com>
Link:      https://skia-review.googlesource.com/930577

[ganesh] Avoid overflow when combining AAHairlineOps

[ganesh] Avoid overflow when combining AAHairlineOps 
 
Bug: b/382786791 
Change-Id: I955d943015cce76f75221df9fab0897a6f22fe4b 
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/930577 
Reviewed-by: Michael Ludwig <michaelludwig@google.com> 
Commit-Queue: James Godfrey-Kittle <jamesgk@google.com>

I'm not entirely sure how to reproduce this. jamesgk@: Could you confirm whether https://skia-review.googlesource.com/930577 fixes this bug please?

jamesgk: Uh oh! This issue still open and hasn't been updated in the last 14 days. This is a serious vulnerability, and we want to ensure that there's progress. Could you please leave an update with the current status and any potential blockers?

If you're not the right owner for this issue, could you please remove yourself as soon as possible or help us find the right one?

If the issue is fixed or you can't reproduce it, please close the bug. If you've started working on a fix, please set the status to Started.

Thanks for your time! To disable nags, add Disable-Nags (case sensitive) to the Chromium Labels custom field.

Yes, that CL fixed the issue.

Security Merge Request Consideration: This is sufficiently serious that it should be merged to stable. But I can't see a Chromium repo commit here,so you will need to investigate what - if anything - needs to be merged to M131. Is there a fix in some other repo which should be merged? Or, perhaps this ticket is a duplicate of some other ticket which has the real fix: please track that down and ensure it is merged appropriately. Security Merge Request Consideration: This is sufficiently serious that it should be merged to beta. But I can't see a Chromium repo commit here,so you will need to investigate what - if anything - needs to be merged to M132. Is there a fix in some other repo which should be merged? Or, perhaps this ticket is a duplicate of some other ticket which has the real fix: please track that down and ensure it is merged appropriately. Security Merge Request - Manual Review: Merge review required: no relevant commits could be automatically detected (via Git Watcher comments), sending to merge review for manual evaluation. If you have not already manually listed the relevant commits to be merged via a comment above, please do so ASAP.

Security Merge Request - Manual Review: Merge review required: no relevant commits could be automatically detected (via Git Watcher comments), sending to merge review for manual evaluation. If you have not already manually listed the relevant commits to be merged via a comment above, please do so ASAP.

Security Merge Request: Thank you for fixing this security bug! We aim to ship security fixes as quickly as possible, to limit their opportunity for exploitation as an "n-day" (that is, a bug where git fixes are developed into attacks before those fixes reach users).

We have determined this fix is necessary on milestone(s): [131, 132].

Please answer the following questions so that we can safely process this merge request:

1. Which CLs should be backmerged? (Please include Gerrit links.)
2. Has this fix been verified on Canary to not pose any stability regressions?
3. Does this fix pose any potential non-verifiable stability risks?
4. Does this fix pose any known compatibility risks?
5. Does it require manual verification by the test team? If so, please describe required testing.
6. (no answer required) Please check the OS custom field to ensure all impacted OSes are checked!

1. https://skia-review.googlesource.com/930577
2. Yes
3. No
4. No
5. No

Reading through this issue, it's unclear if this overflow and OOB write could be realistically exploited in a real world scenario, and if so, would be constrained / limited by scenarios requiring some degree of a patch of a large amount of time, providing reduced attacker control.

Given this, declining merge to branches earlier than 132, especially since Stable and Extended Stable updates are being cut tomorrow or Monday for release right after the current release freeze. Not seeing any issues with this fix on Canary or dev since it was landed, therefore approving merge of this fix (https://skia-review.googlesource.com/c/skia/+/930577) to 132. Please CP and merge to branch 6834 by EOD Monday, 6 January so this fix can be included in the next 132 Beta and RC cut for 132 Stable. Thank you.

Reading through this issue, it's unclear if this overflow and OOB write could be realistically exploited in a real world scenario, and if so, would be constrained / limited by scenarios requiring some degree of a patch of a large amount of time, providing reduced attacker control.

From my understanding, 1) this bug is exploitable 2) this bug can be exploited without any patch, but exploitation takes long.

First, with a compromised renderer, the adversary can craft arbitrary command sending to skia (same as b/360265320), subsequently, a crafted command sequence (skp file we generated) causes int overflow/OOB in the GPU process. This step does not require any patch.

Second, all the patches in this issue aim at accelerating reproduction. This bugs indeed require long time to reproduce (several days in ASan build, it can be shorter for non-ASan release).

Therefore, I consider this bug an exploitable privilege escalation vulnerability (when adversaries have a compromised renderer, they can cause memory corruption in GPU process), but this vuln is mildly mitigated by the significant times required.

I think we are saying the same thing, but you just used more words. :) I agree this could be exploitable, but mitigated to the extent that if exploited - as presented - in a real world scenario, there would be limited attacker control due to the timing and requirement of a compromised renderer. As such, this remains a vulnerability and set a high severity. In terms of backmerge justification, however, this scenario doesn't meet backmerge requirements in this scenario. Therefore, I made my assessment and declined backmerge to older branches and have approved backmerge to current beta branch for inclusion in the forthcoming Stable.

Ok I misunderstood what you mean, thanks for the clarification:)

This issue has been approved for a merge. Please merge the fix to any appropriate branches as soon as possible!

If all merges have been completed, please remove any remaining Merge-Approved labels from this issue.

Thanks for your time! To disable nags, add Disable-Nags (case sensitive) to the Chromium Labels custom field.

The NextAction date has arrived: 2025-01-06 To opt-out from this automation rule, please add Optout-Blintz-Nextaction-Alert to the "Chromium Labels" custom field.

Merged landed in https://skia-review.googlesource.com/c/skia/+/935337

Project: skia
Branch: chrome/m132
Author: James Godfrey-Kittle <jamesgk@google.com>
Link:      https://skia-review.googlesource.com/935337

[ganesh] Avoid overflow when combining AAHairlineOps

[ganesh] Avoid overflow when combining AAHairlineOps 
 
Bug: b/382786791 
Change-Id: I955d943015cce76f75221df9fab0897a6f22fe4b 
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/930577 
Reviewed-by: Michael Ludwig <michaelludwig@google.com> 
Commit-Queue: James Godfrey-Kittle <jamesgk@google.com> 
(cherry picked from commit 8b030e47588af50f56ef380d81a17667baeb582b) 
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/935337 
Reviewed-by: James Godfrey-Kittle <jamesgk@google.com> 
Auto-Submit: Michael Ludwig <michaelludwig@google.com> 
Commit-Queue: Michael Ludwig <michaelludwig@google.com>

LTS Milestone M126

This issue has been flagged as a merge candidate for Chrome OS' LTS channel. If selected, our merge team will handle any additional merges. To help us determine if this issue requires a merge to LTS, please answer this short questionnaire:

1. Was this issue a regression for the milestone it was found in?
2. Is this issue related to a change or feature merged after the latest LTS Milestone?

This issue requires additional review before it can be merged to the LTS channel. Please answer the following questions to help us evaluate this merge:

1. Number of CLs needed for this fix and links to them.
2. Level of complexity (High, Medium, Low - Explain)
3. Has this been merged to a stable release? beta release?
4. Overall Recommendation (Yes, No)

Congratulations Han Zheng! Thank you for your efforts and reporting this issue to us!

Project: skia
Branch: chrome/m126
Author: James Godfrey-Kittle <jamesgk@google.com>
Link:      https://skia-review.googlesource.com/935716

[M126-LTS][ganesh] Avoid overflow when combining AAHairlineOps

[M126-LTS][ganesh] Avoid overflow when combining AAHairlineOps 
 
Bug: b/382786791 
Change-Id: I955d943015cce76f75221df9fab0897a6f22fe4b 
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/930577 
Reviewed-by: Michael Ludwig <michaelludwig@google.com> 
Commit-Queue: James Godfrey-Kittle <jamesgk@google.com> 
(cherry picked from commit 8b030e47588af50f56ef380d81a17667baeb582b) 
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/935716 
Commit-Queue: Gyuyoung Kim (xWF) <qkim@google.com> 
Commit-Queue: Michael Ludwig <michaelludwig@google.com> 
Reviewed-by: James Godfrey-Kittle <jamesgk@google.com>

This bug has been closed for more than 14 weeks. Removing issue access restrictions.